-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
add strip_whitespace option for ConstrainedStr and constr
#163
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #163 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 9 9
Lines 910 918 +8
Branches 202 204 +2
=====================================
+ Hits 910 918 +8 |
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.
Looking good. Please can you:
- update
HISTORY.rst - update docs
- add a
strip_whitespaceoption toConfig
|
@samuelcolvin updated, please check |
HISTORY.rst
Outdated
| @@ -7,6 +7,7 @@ v0.8.1 (2018-XX-XX) | |||
| ................... | |||
| * tweak email-validator import error message #145 | |||
| * fix parse error of parse_date() and parse_datetime() when input is 0 #144 | |||
| * **breaking change**: add ``Config.anystr_strip_whitespace`` and ``strip_whitespace`` kwarg to ``constr``, by default values is `True` #163 | |||
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.
Maybe best to make this False by default an avoid a breaking change.
Since this is easy for people to change and reuse that change throughout their projects I think it would be better to avoid the unnecessary breaking change?
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.
In my experience we always need to strip any string data during validation except passwords or tokens. Also many validation libraries use strip logic enabled by default. But you are the owner and I will do what you prefer.
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.
Let's use False by default, it's better for now. At first we will introduce to people new settings to control strip logic and maybe be in future we will switch to True.
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.
ok, makes sense. This would require a patch release anyway, so let's just go with this.
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.
just waiting for tests to pass.
|
Thanks a lot, I'll try and merge a couple of other PRs which are finished and deploy in the next couple of days. |
|
And I will try to implement more things from #161 😃 |
* feat: add `default_factory` * we will not deepcopy default values * error message for bad default factory * bad default_factory should not raise ValidationError * tiny cleanup Co-authored-by: Samuel Colvin <s@muelcolvin.com>
No description provided.