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

Add "attr.validators.disabled" context manager #859

Merged
merged 8 commits into from
Nov 17, 2021
Merged

Conversation

sscherfke
Copy link
Contributor

@sscherfke sscherfke commented Nov 2, 2021

Pull Request Check List

This is just a friendly reminder about the most common mistakes. Please make sure that you tick all boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing left to do. If your pull request is a documentation fix or a trivial typo, feel free to delete the whole thing.

  • Added tests for changed code.
  • [xx] New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives. Find the appropriate next version in our __init__.py file.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@sscherfke
Copy link
Contributor Author

My first idea was to name the function out_validators():

with out_validators():
    ...

🤪

@sscherfke sscherfke force-pushed the no-run-validators-cm branch 2 times, most recently from 8bf5cb8 to 561addd Compare November 2, 2021 19:09
@hynek
Copy link
Member

hynek commented Nov 3, 2021

hmmm the problem here is that it's extremely not thread-safe? 😅

But I guess @glyph asked for it already in #188 (2017!) so I guess if we document it properly…?

@hynek
Copy link
Member

hynek commented Nov 3, 2021

I think I like his attr.validators.disabled() naming btw!

@sscherfke
Copy link
Contributor Author

hmmm the problem here is that it's extremely not thread-safe? 😅

It’s not more thread-(un)safe than the underlying function set_run_validators() 🙃

I think I like his attr.validators.disabled() naming btw!

I’d have no problem with that, but set_run_validators() and get_run_validators() are defined in _config.py and are made available directly under the attr namespace. So putting no_run_validators() to validators.disable() would be a bit inconsistent, wouldn’t it?

@hynek
Copy link
Member

hynek commented Nov 4, 2021

hmmm the problem here is that it's extremely not thread-safe? 😅
It’s not more thread-(un)safe than the underlying function set_run_validators() 🙃

100% correct, it's just that a context manager looks like it might be so we should nail it home in the doc strings – ideally with a warning block.

I think I like his attr.validators.disabled() naming btw!

I’d have no problem with that, but set_run_validators() and get_run_validators() are defined in _config.py and are made available directly under the attr namespace. So putting no_run_validators() to validators.disable() would be a bit inconsistent, wouldn’t it?

It would. But OTOH I don't expect anyone to use set_run_validators() and get_run_validators() after disable() landed. :D I wouldn't even mind to create better named aliases in the validators namespace and deprecate the old ones (without ever removing them – but not taking them with us into the attrs namespace).

@sscherfke
Copy link
Contributor Author

It has been done.

@sscherfke sscherfke changed the title WIP: Add "no_run_validators()" context manager Add "no_run_validators()" context manager Nov 11, 2021
@sscherfke
Copy link
Contributor Author

Is there anything left to be done?

Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changelog.d/859.change.rst Outdated Show resolved Hide resolved
docs/api.rst Outdated Show resolved Hide resolved
src/attr/_config.py Outdated Show resolved Hide resolved
src/attr/_config.py Outdated Show resolved Hide resolved
src/attr/__init__.pyi Outdated Show resolved Hide resolved
tests/test_validators.py Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
tests/test_validators.py Outdated Show resolved Hide resolved
@sscherfke
Copy link
Contributor Author

Done. Not sure, what kind of example you think of for the typing examples, though.

@sscherfke
Copy link
Contributor Author

"Just a few docs and typing changes. What could go wrong", he thought.

Gonna fix it tomorrow.

@hynek
Copy link
Member

hynek commented Nov 16, 2021

Not sure, what kind of example you think of for the typing examples, though.

Just use the API and see if MyPy complains. :)

@sscherfke
Copy link
Contributor Author

Okay, fixed it.

@hynek hynek enabled auto-merge (squash) November 17, 2021 05:59
Copy link
Member

@hynek hynek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I've done some minor fixes myself. The gh cli is really neat.

@hynek hynek merged commit c6143d5 into main Nov 17, 2021
@hynek hynek deleted the no-run-validators-cm branch November 17, 2021 06:05
@hynek hynek changed the title Add "no_run_validators()" context manager Add "attr.validators.disabled" context manager Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants