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
Further "unknown marks warning" improvements #5178
Conversation
Codecov Report
@@ Coverage Diff @@
## features #5178 +/- ##
============================================
- Coverage 93.14% 90.67% -2.48%
============================================
Files 115 115
Lines 26139 26124 -15
Branches 2578 2576 -2
============================================
- Hits 24348 23687 -661
- Misses 1471 2106 +635
- Partials 320 331 +11
Continue to review full report at Codecov.
|
changelog/5023.feature.rst
Outdated
@@ -0,0 +1 @@ | |||
``--strict`` is now called ``--strict-markers`` as it is more explicit about what it does. The old name still works for backward compatibility. |
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.
With regard to --strict
I would keep it anyway, being able to select also other strict modes later..
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.
And therefore it could still be used in default config (when the intention here is to enforce strict checkings in general).
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.
With regard to --strict I would keep it anyway, being able to select also other strict modes later..
It has not been removed, I only added --strict-markers
as the now preferred name, --strict
is still present and works as before.
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 know.. it just sounded like it would be only kept for backward compatibility, while I think it makes sense to keep it in general.
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.
And --strict
should be used in some test probably still.
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.
Indeed, I thought the same thing, please see test_strict_prohibits_unregistered_markers
I know.. it just sounded like it would be only kept for backward compatibility, while I think it makes sense to keep it in general.
Not sure if we should ever change the meaning of --strict
in the future as well. I mean, suppose we decide to turn "cannot collect classes with __init__
" into an error using --strict
. If we do that, then test suites which used --strict
to turn marks into errors will now break.
In the future we probably should grow another option for that kind of check and leave this option alone?
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.
@blueyed any thoughts on this? If you feel strongly about changing the meaning of --strict
and wording, I don't mind changing it back.
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.
If we do that, then test suites which used
--strict
to turn marks into errors will now break.
I'd argue that this is OK - and easy to fix then.
It is easier to have --strict
for CI etc, than having to add multiple --strict-*
variants then.
New strictness checks could still get their own option, of course (if it makes sense).
doc/en/reference.rst
Outdated
markers = | ||
slow | ||
serial | ||
|
||
**Note**: This option was previously called ``--strict``, which is now an | ||
alias preserved for backward compatibility. |
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.e. this could be rephrased.
@blueyed gentle ping. 😁 |
I'm fine with keeping it like this, but having it clearer would allow problems with deprecations in the future. I.e. if we say now that |
Makes sense, I don't have strong feelings either way to be honest. Can you suggest the changes you would like to see then (either here as "suggestions" or a commit yourself)? |
It doesn't seem to add much value (why would one execute tests based on that marker?), plus using the docstring for that encourages one to write a more descriptive message about the test
b452681
to
0594dba
Compare
Went ahead and implemented the suggested changes to the docs, please let me know what you think. 👍 |
Fix #5023