Fixed bug in inpkg where generated source would import itself

[tamccall]

Fixes import cycle bug when doing inpkg generation

See: #141

[tamccall]

@codeactual @evanphx could i get you guys to take a look at this? Trying to get master into a better state

[codeactual]

Future commits for #144 probably belong in this PR instead because the new tests would verify the fix.

Does inpkg need more coverage, unrelated to import cycles, to assert its general behavior after this patch?

One initial thought on the commit: some comments on the changed if lines that were changed might be helpful, e.g. "X always needs to be compared to Y to prevent import cycles", etc. in case those lines need to be revisited later for some other purpose.

[tamccall]

@codeactual

Does inpkg need more coverage, unrelated to import cycles, to assert its general behavior after this patch
I was mainly refering to this issue

X always needs to be compared to Y..
I'll add some comments

Future commits for #144 probably belong in this PR instead because the new tests would verify the fix.
are you suggesting that we hold off on the fix until we add enough testing to verify it?

[codeactual]

I was mainly refering to this issue

I was curious if, in your opinion, more tests are needed in general to reduce inpkg regressions (that might even result from this fix). I've only taken a closer look a very small portion of the GeneratorSuite, so I don't have an opinion. This issue raised the question of whether #141 was sort of bound to happen at some point because there wasn't good enough coverage historically.

are you suggesting that we hold off on the fix until we add enough testing to verify it?

I think there's plenty of time for it.

You were already quick to provide a patch for anyone with an urgent need. I think it's reasonable for everyone else to wait for a verified one.

[tamccall]

added a test to catch the import cycle

[tamccall]

I was curious if, in your opinion, more tests are needed in general to reduce inpkg regressions (that might even result from this fix). I've only taken a closer look a very small portion of the GeneratorSuite, so I don't have an opinion. This issue raised the question of whether #141 was sort of bound to happen at some point because there wasn't good enough coverage historically.

I do agree with the statement that "#141 was sort of bound to happen at some point," so adding additional testing for inpkg generation is likely needed. However I am not sure how we would want to go about adding those tests. There are about a million different cases that are being tested by that suite, so I certainly wouldn't advocate for verifying every case again for inpkg mode.

What might make more sense to me would be adding an inpkg run to the integration target and asserting that the package still builds after running mockery

[codeactual]

Yep. I was not advocating to multiply the coverage or something ("million different cases") for the inpkg scenario.

This was my only concern:

I was curious if, in your opinion, more tests are needed in general to reduce inpkg regressions (that might even result from this fix).

If the new test is sufficient, that is ideal.

[evanphx]

Sorry about the delay! Thanks for the fix