Skip to content
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

Make --strict imply --strict-config / xfail_strict? #7503

Open
The-Compiler opened this issue Jul 15, 2020 · 36 comments
Open

Make --strict imply --strict-config / xfail_strict? #7503

The-Compiler opened this issue Jul 15, 2020 · 36 comments
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Milestone

Comments

@The-Compiler
Copy link
Member

The-Compiler commented Jul 15, 2020

With pytest 4.5.0 (May 2019), we introduced --strict-markers as a replacement for --strict, and said:

The existing --strict option has the same behavior currently, but can be augmented in the future for additional checks.

With #7286, now we also have --strict-config. Yet, --strict is still an alias for --strict-markers.

Given that pytest 6 is allowed to do breaking changes, wouldn't it make sense for --strict to imply both --strict-markers and --strict-config now?

(Though, on a second thought: Do we actually need --strict-markers in pytest 6 still, given the warning system? Probably still useful as a convenient way to turn those warnings into errors?)

This would also make pytest more in line with other tools with a --strict flag - mypy and tsc (the typescript compiler) come to mind, where --strict implies various different strict flags.

cc @gnikonorov (who added --strict-config), @webknjaz and @DahlitzFlorian (some existing discussion in #7233), and @nicoddemus (#5023) - what do you think?

@webknjaz
Copy link
Member

webknjaz commented Jul 15, 2020

I think it's a good idea to have some --strict-all-the-things that would imply enabling all strictness-related settings. What I'm not sure about is how it should be called. I'm okay with just --strict but Bruno mentioned earlier that it used to mean just "strict-markers" for a while so such semantics change may be confusing. OTOH personally I can live with that and I'm not really concerned about this name. I think it should be decided by the folks who feel differently.

@webknjaz
Copy link
Member

The only thing I want to add is that I'd probably expect it to also be a config value rather than just a CLI option.

@Zac-HD Zac-HD added the type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature label Jul 15, 2020
@Zac-HD
Copy link
Member

Zac-HD commented Jul 15, 2020

I like the idea of --strict or strict = True implying all the strictness options.

Whether we still need it with the warnings system or not, I think we should keep it until pytest 7 rather than scrambling to make such a large change now.

@DahlitzFlorian
Copy link
Member

DahlitzFlorian commented Jul 15, 2020

I really like the idea of using --strict to activate all --strict* options. However, I would suggest adding a note to the documentation that --strict will have that behavior in version x.x and people should be aware of it. Of course, we already added a note that it might happen, but an explicit note that it is going to happen in version x.x is highly preferred.

IMHO we can discuss whether we need to wait until version 7 or can include it in some minor version of pytest 6.

@symonk
Copy link
Member

symonk commented Jul 15, 2020

Should we consider giving the user the option?

--strict=all | --strict=marker | --strict=config

Im just wondering if we add future 'strictness' but have then coupled the user into all or none situations.

@The-Compiler
Copy link
Member Author

@symonk We already have --strict-markers and --strict-config, I don't think we should have a second way to do the same thing.

@DahlitzFlorian
Copy link
Member

Totally agree with @The-Compiler. In fact, --strict should be a shortcut for using all --strict* options.

@gnikonorov
Copy link
Member

No objections from me at all. --strict should imply all strict flags IMO, as @DahlitzFlorian mentions above.

@The-Compiler
Copy link
Member Author

I opened a PR: #7529

@graingert
Copy link
Member

Given that pytest 6 is allowed to do breaking changes

Can we flip the default here so markers default to strict in v6?

What about xfail_strict

@The-Compiler
Copy link
Member Author

Can we flip the default here so markers default to strict in v6?

We already have a warning for that since 4.5.0, so it should be quite clear what's going on even without --strict-markers. -1 on making that the default now, that seems a bit late.

What about xfail_strict

That one is much more controversial and I'm not sure if starting the discussion again will produce a different outcome. See #1299 and #814.

@graingert
Copy link
Member

We could then change the default to True in a future release so pytest by default will be more strict

From @nicoddemus seems like this is exactly that future release

@graingert
Copy link
Member

I think if we add strict-markers and strict-config to --strict we should also have it cover xfail_strict

@The-Compiler
Copy link
Member Author

I think if we add strict-markers and strict-config to --strict we should also have it cover xfail_strict

While I love xfail_strict (and enable it in all my projects), I think it's fine for --strict to imply any strict commandline arguments, but not anything outside of that. If it was an --xfail-strict flag I'd agree, but since it's a setting in the config, I don't think we should do this now (and after the RC is out already).

@webknjaz
Copy link
Member

Having xfail_strict stand aside makes me puzzled... Honestly, it's weird that some of the strict options are config-only and others are CLI-only (#7233). They are all part of the same settings category IMHO and it'd be great to treat them in the same way, w/o special-casing things...

@graingert
Copy link
Member

I don't think we should do this now (and after the RC is out already).

In that case I think we should just drop the --strict alias in v6.0 to make it semver-available as a comprehensive flag for all 3 modes in V6.1

@nicoddemus
Copy link
Member

In that case I think we should just drop the --strict alias in v6.0 to make it semver-available as a comprehensive flag for all 3 modes in V6.1

Indeed I believe this is the safest option: the breaking change in 6.0 then becomes the fact that --strict gets removed, and then we can reintroduce it in a minor release (6.1 for example). This seems better to me than changing the behavior of an existing option, because a missing option is a hard error, the former is a change in behavior which might be harder to track down if one does not read the CHANGELOG. Of course it might happen that an user skips directly from 5.4 to 6.1 and gets the behavior change directly, but still seems like it is less likely to cause problems.

As for if it should also cover xfail_strict, I think for the sake of avoiding confusion, --strict should also cover xfail_strict, or for consistency we don't reintroduce --strict as a command-line option but as a config option instead.

@webknjaz
Copy link
Member

As for if it should also cover xfail_strict, I think for the sake of avoiding confusion, --strict should also cover xfail_strict, or for consistency we don't reintroduce --strict as a command-line option but as a config option instead.

Could also introduce --xfail-strict...

@The-Compiler
Copy link
Member Author

Can we remove --strict in pytest 6.0 without actually having it deprecated (as in, not just discouraged but showing a warning)?

@nicoddemus
Copy link
Member

Could also introduce --xfail-strict...

I don't see much value in it being a command-line value, TBH. To me it feels like a test-suite wide configuration rather something you want to switch on/off in the command-line.

Can we remove --strict in pytest 6.0 without actually having it deprecated (as in, not just discouraged but showing a warning)?

Hmm good point! Indeed we just discouraged it in the docs.

To follow proper protocols we should:

6.0.0: formally deprecate --strict with PytestDeprecationWarning.
7.0.0: remove --strict altogether.
7.x.0: reintroduce --strict with different semantics.

Note the same applies to changing the meaning of --strict (the original proposal) because we need also a deprecation period:

6.0.0: issue a PytestWarning that --strict will change behavior in 7.0.
7.0.0: change --strict behavior.

I believe the latter causes less friction. Thoughts?

@The-Compiler
Copy link
Member Author

Okay, so I suppose let's only deprecate --strict for 6.0 then? I don't see the point in removing it in a release just to reintroduce it again, that just makes things more confusing for everyone.

I'd be glad if someone else could take over here though, as I should take care of some other things at the moment.

@graingert
Copy link
Member

graingert commented Jul 22, 2020

Time for a plugin: pytest-strictest to introduce "--strictest" as a command flag that enables all 3 strict options

@graingert
Copy link
Member

graingert commented Jul 22, 2020

fwiw I like the --strict=all suggestion that enables all 3 strict modes

@nicoddemus
Copy link
Member

nicoddemus commented Jul 22, 2020

Okay, so I suppose let's only deprecate --strict for 6.0 then? I don't see the point in removing it in a release just to reintroduce it again, that just makes things more confusing for everyone.

I think so yes. Created #7530 for that.

Thanks everyone for participating in the discussion. I've moved this issue to the 7.0 milestone.

@nicoddemus nicoddemus removed this from the 6.0 milestone Jul 22, 2020
@nicoddemus nicoddemus added this to the 7.0 milestone Jul 22, 2020
@The-Compiler
Copy link
Member Author

I suppose this could be closed as a duplicate of #7233 then?

@nicoddemus
Copy link
Member

Good point, but I think we should rather close #7233 and keep this one open, as I believe this discussion is more through and brought more ideas on how to move things forward. 👍

@The-Compiler The-Compiler changed the title pytest 6: Make --strict imply --strict-config? Make --strict imply --strict-config / xfail_strict? Jul 23, 2020
@graingert
Copy link
Member

graingert commented Jul 5, 2021

what about passing a pypi major version to strict: --strict=7 any new strictness settings will be graded with the major version they are added in, then --strict will enable any flags that are greater than that version.

Then people can pass --strict=inf to opt into all strictness settings

@graingert
Copy link
Member

then you can keep --strict with no = deprecated forever

@The-Compiler
Copy link
Member Author

I'm not sure what problem that would solve. Major releases come with some breakage, so projects likely need to adapt a little anyways - I don't see an issue with making --strict imply ~all strict settings and adding to it, especially in major releases. This just seems like very unusual UX and a solution without a problem IMHO 😉

@graingert
Copy link
Member

Usually new strict options get added in minor releases because they're not a breaking change. If we made --strict a recommendation then release managers would have to delay every new flag to a new major release

@RonnyPfannschmidt
Copy link
Member

Having new strict flags trigger warnings until the next major release seems like a good idea anyway

Having strict simply error on all from the last major release and warn on the new ones seems like a nice plan to me

@The-Compiler
Copy link
Member Author

I don't follow. The whole point of the strict flags (at least the ones we have so far) is to turn warnings into errors?

@graingert
Copy link
Member

You're saying Pytest 7 should just enable strict-config and strict-markers by default?

@The-Compiler
Copy link
Member Author

I'm not sure where you see me saying that. I'm saying --strict should imply both --strict-config and --strict-markers, with us being free to add more strictness settings over time if needed.

@graingert
Copy link
Member

My usecase/workflow is that I want to opt into all breaking warnings and exceptions and strictness settings and then dependabot will make a new PR where I manually disable any errors I can't fix immediately

@The-Compiler
Copy link
Member Author

The-Compiler commented Jul 6, 2021

For the unlikely situation that --strict with a new pytest version implies --strict-frobnication and you can't fix that right away, you could always replace --strict by --strict-config --strict-markers temporarily. It's not like we're going to have hundreds of new strict options suddenly. I feel like we're thoroughly overengineering something simple at this point.

@bluetech bluetech modified the milestones: 8.0, 9.0 Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: proposal proposal for a new feature, often to gather opinions or design the API around the new feature
Projects
None yet
Development

No branches or pull requests

10 participants