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
bottle: add tests for writing part #9490
Conversation
Review period will end on 2020-12-10 at 22:06:30 UTC. |
@SeekingMeaning this will conflict with your changes in #9095. I would like to get my changes in first, so I can proceed on working on the next steps for #9315. The pattern refactoring can then come as a second step. |
Just if someone wonders: I struggled with the flush/closes of the files. The current code was the only way I found to make it work. |
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.
This refactoring is now very hard to review.
I'm afraid I just don't have confidence that this isn't going to break code that's very important and widely used to "improve readability" in a way that I don't think actually does improve readability. I worry that it's only more readable to you @iMichka now because you're the one who has refactored it.
For changes like this what I'd propose is:
- open a PR that adds tests to cover 100% of the existing code you wish to refactor and makes no modifications to the existing code. This will demonstrate the tests exercise (at least some) of the existing functionality.
- open a PR that refactors the existing code and makes no modifications to the tests. This will demonstrate that the refactor does not change (at least some) of the existing functionality.
- additional, small PRs can be made that add more tests and refactor without needing to alter existing tests.
Sorry to do this, I've just seen these refactorings usually break things (sometimes in ways that aren't apparent until months down the line).
I would like #9095 to land before this as it's fixing a problem that keeps reoccurring whereas this PR does not in itself do anything beyond refactoring code. |
Review period ended. |
0d5a0dc
to
2896b29
Compare
2896b29
to
5a796f8
Compare
@MikeMcQuaid I tried a different strategy. Can you have a quick look to validated the approach before I add more tests?
|
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.
Looking great, thanks for the changes @iMichka! This will definitely provide more confidence while refactoring here 👏🏻
a60abdb
to
459015e
Compare
459015e
to
a408461
Compare
The test is passing now, but I can't make brew style pass. Either brew style, either the test is fine. I can't make both work together ... |
ef66296
to
8411c2a
Compare
8411c2a
to
bf61134
Compare
This should work now. Thanks @SeekingMeaning for the test fix! |
bf61134
to
932d45a
Compare
932d45a
to
af09563
Compare
af09563
to
73c0c92
Compare
In preparation for Homebrew#9315
73c0c92
to
4afcae5
Compare
Merged, CI is green. I have one or two more tests to test other branches of that part of the code, this should be straightforward now and I will open a new PR for that. |
Thanks for persevering @iMichka! |
In preparation for #9315
brew style
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?