-
Notifications
You must be signed in to change notification settings - Fork 761
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
class library: Pfset - do cleanup on nil stream #4815
class library: Pfset - do cleanup on nil stream #4815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks fine to me.
We make it a general practice now to add a unit test for bugfixes whenever practical. This one is easy: I'd add a test method to TestPattern. I think it will be enough to request one event from an empty Pfset, where the init sets a flag to false and the cleanup sets it to true. Then there are 3 final states:
- true: pass
- false: fail (init ran, cleanup didn't)
- nil: fail (neither init nor cleanup ran)
2 asserts would be enough to distinguish these.
I'm not at the computer now, haven't tried it, but I think that will fly.
I've done something a bit more elaborate in terms of testing
Let me see if can add this to the pull request(s) without screwing up the actual fix... I've done a push of the test; it seems to be showing up below. |
The documentation could probably use some spiffing up to explain the behavior. Presently it just says something rather vague about the inti function
No promise to execute anything else in it (meh), or basically to even call it if there's nothing to do in the event, i.e if it's nil. But (amusingly) it did basically promise it would run cleanup on nil ("when the supplied pattern runs out of values"):
So based on that I'm punting on updating the documentation right now; only the init stuff needs a bit of clarification and it can be considered an unrelated issue to this bugfix... Aside: I see there's some issue with one of the CI servers (something about lint mismatching clang version). I'm pretty sure I didn't cause that. Hopefully it will resolve itself... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test logic looks OK. A few style suggestions.
…st_Pfset_..._empty_stream
….test_Pfset_..._empty_stream per code review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok now.
Oh btw... "I see there's some issue with one of the CI servers (something about lint mismatching clang version). I'm pretty sure I didn't cause that." The class library isn't linted, so no, you didn't cause it :D
thanks! @jamshark70 would this be a breaking change? can it go into 3.11.x? |
I don't think it's a breaking change -- I'm pretty sure the expectation was to run the cleanup function no matter how many events were yielded in the interim (including 0). Also I don't recall this class being frequently mentioned -- I think it isn't used that much. |
Purpose and Motivation
Fixes #4807 Pfset doesn't do cleanup if its sub-pattern yields nil immediately.
Types of changes
To-do list