-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat: add --require-unique-paramset-ids option skips pytest internal … #13757
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
feat: add --require-unique-paramset-ids option skips pytest internal … #13757
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.
Should i documentation like there is for --collect-only?
Yes please - and then I think we'll be ready to merge 🙂
907cf3e
to
8a770c9
Compare
Is this till needed or did with @nicoddemus say cover this? |
c0ce285
to
a07ff8a
Compare
35da7df
to
4d6fa15
Compare
71152b2
to
9cebbab
Compare
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.
Done all fixes
@Zac-HD just realized something: doesn't make more sense for this to be an ini option rather than a command-line flag? I understand this is something one would like to setup permanently for a test suite, rather than something to be passed on the command line on occasion. |
Ideally both, I guess? |
b1bae36
to
b825eb4
Compare
@nicoddemus, @Zac-HD |
03b31f8
to
358627e
Compare
358627e
to
f547106
Compare
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.
Minor edits to the docs, and then I'll give @nicoddemus a few days to comment, and then merge!
FWIW I agree with this, I don't see a reason for this to be a CLI flag, only ini. Having fewer flags is generally a good thing. In the future we could also consider adding it to the planned |
I understand wanting to have fewer flags to reduce complexity and setup pain. Because we can get the same results from an ini and most likely, this parameter wills be set at the level of the whole execution and not just a few tests being run Would you like me to remove the cli flag oart of this pr and leave the ini? |
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.
Some comments from me. I think this can be a nice strictness flag.
I'll let @Zac-HD decide, I don't feel strongly about it. |
57b5f00
to
9a1d479
Compare
@bluetech I went over and fixed all your comments, thanks for the input. Main changes made:
|
9a1d479
to
4782ea6
Compare
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.
Thanks for the update! Please see my comments below.
BTW, if we go with ini only, I think it would make sense to use the name |
9d7870a
to
4eaf20e
Compare
4eaf20e
to
767a11f
Compare
@bluetech Fixed all the comments is there anything else you think needs doing? |
@gomri15 Instead of another round of pedantic comments, I pushed a commit with the changes I thought about. Some comments:
Please take a look at the changes and let me know what you think. If it's good I can merge. |
id if id is not HIDDEN_PARAM else "<hidden>" for id in resolved_ids | ||
) | ||
duplicates = ", ".join( | ||
id if id is not HIDDEN_PARAM else "<hidden>" |
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.
not a blocker or a reason not to merge.
this is a pattern that does repeat itself a few times in the code
id if id is not HIDDEN_PARAM else "<hidden>"
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 use https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) for this :)
It's a little repetition but close together and unlikely to get out of sync. So I prefer making the code more readable.
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.
Haha i like it 2 is just under 3 haha
@bluetech Thank you very much for getting this a cross the line, really appreciate your input and I'll make a point of growing from it. the changes look good to me. |
cca6f1c
to
5df54c6
Compare
Merged, thanks @gomri15 it was a great first PR. I will definitely use this option in my own projects. |
Thank you @bluetech Pleasure working with you |
Closes #13737