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 strip_whitespace option for ConstrainedStr and constr #163

Merged
merged 1 commit into from Apr 24, 2018

Conversation

@Gr1N
Copy link
Contributor

@Gr1N Gr1N commented Apr 23, 2018

No description provided.

@codecov
Copy link

@codecov codecov bot commented Apr 23, 2018

Codecov Report

Merging #163 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #163   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         910    918    +8     
  Branches      202    204    +2     
=====================================
+ Hits          910    918    +8

Copy link
Owner

@samuelcolvin samuelcolvin left a comment

Looking good. Please can you:

  • update HISTORY.rst
  • update docs
  • add a strip_whitespace option to Config

@Gr1N Gr1N force-pushed the string-strip branch from c468bfe to 2734c9c Apr 24, 2018
@Gr1N
Copy link
Contributor Author

@Gr1N Gr1N commented Apr 24, 2018

@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
Copy link
Owner

@samuelcolvin samuelcolvin Apr 24, 2018

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?

Copy link
Contributor Author

@Gr1N Gr1N Apr 24, 2018

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.

Copy link
Contributor Author

@Gr1N Gr1N Apr 24, 2018

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.

Copy link
Owner

@samuelcolvin samuelcolvin Apr 24, 2018

ok, makes sense. This would require a patch release anyway, so let's just go with this.

@Gr1N Gr1N force-pushed the string-strip branch from 2734c9c to bea4a93 Apr 24, 2018
Copy link
Owner

@samuelcolvin samuelcolvin left a comment

just waiting for tests to pass.

@samuelcolvin samuelcolvin merged commit f88e592 into samuelcolvin:master Apr 24, 2018
2 checks passed
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Apr 24, 2018

Thanks a lot, I'll try and merge a couple of other PRs which are finished and deploy in the next couple of days.

@Gr1N Gr1N deleted the string-strip branch Apr 24, 2018
@Gr1N Gr1N mentioned this pull request Apr 24, 2018
@Gr1N
Copy link
Contributor Author

@Gr1N Gr1N commented Apr 24, 2018

And I will try to implement more things from #161 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants