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

Rename isupper, islower, ucfirst and lcfirst #26442

Merged
merged 2 commits into from
Mar 15, 2018
Merged

Rename isupper, islower, ucfirst and lcfirst #26442

merged 2 commits into from
Mar 15, 2018

Conversation

nalimilan
Copy link
Member

@nalimilan nalimilan commented Mar 13, 2018

Spell "uppercase" and "lowercase" in full for consistency with uppercase and lowercase functions.

The current function names seem to be inspired from different languages (Python and Perl), and are therefore inconsistent. Here's a small survey of what other languages use:

This PR: uppercase lowercase isuppercase islowercase uppercasefirst lowercasefirst
Python upper lower isupper islower capitalize
Ruby upcase downcase capitalize
Perl uc lc ucfirst lcfirst
Java toUpperCase toLowerCase isUpperCase isLowerCase StringUtils.capitalize StringUtils.uncapitalize
Rust to_uppercase to_lowercase is_uppercase is_lowercase
Swift uppercased lowercased long* long*
R toupper tolower Hmisc::capitalize

* long: CharacterSet.uppercaseLetters.contains(c) and CharacterSet.lowercaseLetters.contains(c).

@nalimilan nalimilan added the domain:strings "Strings!" label Mar 13, 2018
@JeffBezanson
Copy link
Sponsor Member

Since we use C names for character predicates, maybe we should keep isupper and use toupper and tolower.

HISTORY.md Outdated
@@ -516,7 +516,7 @@ Deprecated or removed
[#18558], [#19711], [#19712], [#19791], [#19802], [#19931], [#20543], [#20228]).

* All methods of character predicates (`isalnum`, `isalpha`, `iscntrl`, `isdigit`,
`isnumber`, `isgraph`, `islower`, `isprint`, `ispunct`, `isspace`, `isupper`,
`isnumber`, `isgraph`, `islowercase`, `isprint`, `ispunct`, `isspace`, `isuppercase`,
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The history file should not be changed.

@JeffBezanson JeffBezanson added the kind:deprecation This change introduces or involves a deprecation label Mar 13, 2018
@nalimilan
Copy link
Member Author

Since we use C names for character predicates, maybe we should keep isupper and use toupper and tolower.

That's a possibility and it would give shorter identifiers. However the "to" prefix doesn't convey very useful information, and AFAIK we don't use it anywhere else.

@nalimilan
Copy link
Member Author

Another advantage of using "case" everywhere is that it's consistent with the titlecase function, which can hardly be renamed to title.

@JeffBezanson JeffBezanson added the status:triage This should be discussed on a triage call label Mar 13, 2018
@StefanKarpinski
Copy link
Sponsor Member

We're increasingly deviating from the C names for these and the C names are pretty bad to begin with. I think we should just pick a good set of names for these and be guided by other high-level languages like Python and Ruby (and Swift, etc.).

@JeffBezanson
Copy link
Sponsor Member

Ok, I'm fine with the names in this PR. Just throwing out some options.

Spell "uppercase" and "lowercase" in full for consistency with uppercase and lowercase functions.
@JeffBezanson JeffBezanson removed the status:triage This should be discussed on a triage call label Mar 14, 2018
@JeffBezanson JeffBezanson merged commit e050ab6 into master Mar 15, 2018
@JeffBezanson JeffBezanson deleted the nl/case branch March 15, 2018 04:12
@coveralls
Copy link

Coverage Status

Coverage decreased (-60.5%) to 30.381% when pulling f79b469 on nl/case into f354532 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:strings "Strings!" kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants