-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
allow predicate functions to lstrip etc. and default to stripping unicode whitespace #27309
Conversation
[`isspace`](@ref) for precise details. | ||
|
||
The optional `chars` argument specifies which characters to remove: it can be a single character, | ||
vector or set of characters, or a predicate function. |
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.
The predicate function should always go first per our conventions, documented in the style guide in the manual.
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.
It is a "general rule" not a "must". Having the function argument second here seems much more natural to me.
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.
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 disagree wholeheartedly; it seems very unnatural to me.
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 don't think we should deviate from our argument order conventions for this case.
I agree with @ararslan here: predicate function as the first argument, character sets as the second; we can also allow a predicate as either the first or second argument as the user prefers since a string argument isn't callable. |
Note that we do allow predicate function as a second argument to |
That should probably be fixed. |
I'm also with @ararslan. It doesn't really bother me that the predicate and the chars are in different places in the argument list. |
I'm also OK with adding a predicate-as-first-argument version and just leaving the others. |
@simonbyrne, your PR, your call. Just do whatever you see fit and merge it once CI passes. |
Can I make one last plea to put the function argument first? |
Seems there was broad support for at least allowing that. |
remove last use of _default_delims.
@ararslan I am sympathetic, and I have thought a lot about it, but my opinion is that:
So my call is to merge this PR. |
Also, I got rid of the other uses of |
I very adamantly disagree with this, since the argument order is then not consistent. This will be a case of "functions always come first, except for this one weird case." We had that with
I can submit a PR to add it once this is merged, but it really makes more sense to me to just have I do appreciate your work here and that you've given a lot of thought to this--I don't want you to think otherwise. |
PR to fix the argument order: #27605 |
|
This is similar to #27232, except that the predicate function is the second argument. It does not require any deprecations.
Fixes #27211.