Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Skip the checks if the news file was changed #337
Skip the checks if the news file was changed #337
Changes from 8 commits
4ff07b8
e83c573
d5db270
67f9802
5b4f1b3
4af8fa9
aa34594
9f70599
26ee5ef
de86e40
2d9f9d6
44164e2
b325bcb
1ad22c8
6d34234
814f5ca
6902272
831e6e7
1ff0428
f100f7a
90005e1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 looks like duplicate code between tests.
Why not reuse the existing
create_project
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 dislike
create_project()
.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.
Just to be clear: I'm not criticizing you, I'm criticizing a specific decision you made because I think I know how to solve that specific problem better.
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.
@hexagonrecursion don't worry :) If you think that some code is not ok. please say it loud.
I will not take it as an offense... I know that I am capable of writing bad code :)
Your comments are valid.
Regarding the risky change. As long as the tests are not failing, I think that any change should be done if we thing that it improves the readability and usability.
We should be confident that the test suite is solid and don't code with fear :)
If you think that
create_project
is bad pattern, it's best to add a docstring comment to it and let other developers know what is the current best practice.But I think that all test helpers should have short docstrings describing when and why they should be used :)
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.
Same here. I also prefer to err on the side of not offending anybody.
If you are onboard with my argument I'll submit a followup PR to refactor the rest of the file and delete
create_project()
👍
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.
Do we need this ignore here?
I guess that this is a replacement for
write("newsfragments/123.feature", "Foo the bar")
For me, writing an explicit news fragment is easier to read and understand what is going on here.