ENH: make too many/too few empty lines more verbose #416
Conversation
|
@goanpeca , any thoughts on this tweak to the linter's end of line check? |
|
ehhh sure why not. but tests should be fixed, no? |
|
Yep, you're right. It will need test updates and possibly more tests to ensure we are covering both branches (though I thought we covered both cases before). |
|
Before digging into this too much more, could you help us understand why this is needed, @CJ-Wright ? |
|
I guess I had ran into the case where I had too many lines and didn't know it and was a bit confused by the message for a little while. The idea was that we have the data needed to make a more verbose comment and it doesn't cost us much to write it out so we might as well be more verbose, it could save someone the confusion over of they have too few or too many lines. In retrospect we could even output exactly how many extra/fewer lines they have, although that was not in this PR. |
I did not pursue this originally (I introduced the change you are improving) because of the way we run the test cases (we expect a fixed message in the tests not a variable one...) but I guess we can tweak the tests a bit to make this work. Also @CJ-Wright you need to update the previously mentioned tests. |
|
Yep, I'm ok with this sort of change. Just need to update the tests. |
|
@jakirkham this way of running tests is turning a bit cumbersome. We should probably turn these messages into proper warning/exceptions classes so it is easier to test and further develop. Thoughts? |
|
Seems like a good idea. Could you please add it to a new issue? |
|
If you are using pytest you might also look at pytest.mark.parametrize |
|
@CJ-Wright, what I meant is that we can no longer user messages and test based on that. We need to move to warnings/exceptions. So this is not a refactor for the tests only, but for how the code is written. Hence, pytest.mark.parametrize does not really help. |
|
Sort of tangential, PR ( #385 ) would switch us to |
|
cool I really prefer pytest over unittest |
| if content == valid_content: | ||
| self.assertNotIn(expected_message, lints) | ||
| else: | ||
|
|
|
Excepting one small nit. This looks fine to me. @goanpeca, do you have any more thoughts? |
|
LGTM 👍 |
|
Once the nit is fixed, @CJ-Wright , will happily merge. |
|
Thanks @CJ-Wright. This will be in |
|
Have released this to the linter webservice. So we should start seeing warnings for issues like this. |
|
Thank you @jakirkham ! |
No description provided.