Skip to content

Fix local variable affecting shell environment outside of function #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 18, 2022
Merged

Fix local variable affecting shell environment outside of function #73

merged 2 commits into from
Dec 18, 2022

Conversation

h3xx
Copy link
Contributor

@h3xx h3xx commented Dec 16, 2022

After tab-completing, the $words array would persist and possibly affect other things.

Using local keeps it inside the function scope.

After tab-completing, the `$words` array would persist and possibly
affect other things.

Using `local` keeps it inside the function scope.
@h3xx h3xx changed the title Fix local variable affecting shell environment Fix local variable affecting shell environment outside of function Dec 16, 2022
@ktomk
Copy link
Collaborator

ktomk commented Dec 16, 2022

@h3xx I think the fix is legit, can you please amend with an update of the test-fixture, too?

@h3xx
Copy link
Contributor Author

h3xx commented Dec 17, 2022

Yep, I'll fix it!

However, in the spirit of Festivus (Dec 23), I want to air my grievances about the test that broke. I hope you take this rant as a good-natured, if a little punchy, way for me to speak to my own development and unit testing experience. I don't wish to demean the project or anyone involved; I think it's awesome this project is so well tested. :-)

It is a little concerning that the test that failed depends on a byte-for-byte comparison against a copy of the code it tests.

I recall an article I read a long time ago about a dev who built a unit test that contained an exact copy of the code being tested, a particularly tricky algorithm. The dev said they were okay with updating it twice (once in the source and once in the unit test) stating it was worth 100% code coverage.

Reasons not to do this:

1. It doesn't actually test the code for correctness.

Since it's just a simple text diff, it doesn't matter how the code works, just that it's the same.

Furthermore, if there's a bug in both the source and the copy, the test won't show it - in fact, it'll pass! Instead of testing for the presence of bugs, this test actually presents a barrier to fixing the bug since it must now be fixed in multiple places.

(I'm sure there are other tests that test for correctness, but my point is that this one doesn't.)

2. It deliberately violates DRY (Don't Repeat Yourself).

The code has to exist in two places, and must be updated in both places.

I suppose this could be fixed by copying the source file into the cache before starting, but that IMO would be equally silly.

3. It's extra development overhead.

It's an extra step to making changes, and can be confusing to new developers (it confused me).

It can also muddy the intent of commits in the git history; it may be harder to grok the change intended by the dev, if the change is made n times instead of just one. It's the same reason I avoid committing auto-generated files alongside the files they were generated from - just having one source of truth is best.

...Anyway, back to hacking.

@ktomk
Copy link
Collaborator

ktomk commented Dec 17, 2022

@h3xx Thanks for your support, and thank you especially for sharing your understanding of software maintenance, it's not often we find it.

And yes, we were aware of this, at least in part, automated tests do what they do, and they were written by their authors to their best knowledge at that time. It would derail this PR, so if I may ask if you have interest to discuss this further, to extract the rant to a new issue and if we're good we can get first pain points out of the door after recovering the test-suite which I would like to look into beforehand to be educated for the discussion.

@ktomk ktomk merged commit 83705be into bamarni:master Dec 18, 2022
@ktomk
Copy link
Collaborator

ktomk commented Dec 18, 2022

@h3xx thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants