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 downcase option to String#camelcase #7717

Merged
merged 4 commits into from May 1, 2019

Conversation

@wontruefree
Copy link
Contributor

commented Apr 24, 2019

resolves #7709

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Just lower would be enough. Both variants (lower camel case, upper camel case) should be documented, too.

@@ -3503,10 +3503,10 @@ class String
# "eiffel_tower".camelcase # => "EiffelTower"
# "isolated_integer".camelcase(Unicode::CaseOptions::Turkic) # => "İsolatedİnteger"
# ```
def camelcase(options : Unicode::CaseOptions = Unicode::CaseOptions::None)
def camelcase(options : Unicode::CaseOptions = Unicode::CaseOptions::None, lowercase_first_letter = false)

This comment has been minimized.

Copy link
@konovod

konovod Apr 25, 2019

Contributor
Suggested change
def camelcase(options : Unicode::CaseOptions = Unicode::CaseOptions::None, lowercase_first_letter = false)
def camelcase(options : Unicode::CaseOptions = Unicode::CaseOptions::None, *, lowercase_first_letter = false)

to prevent just false as argument, suggested in #7709 (comment)

This comment has been minimized.

Copy link
@vladfaust

vladfaust Apr 25, 2019

Contributor

I'd stand for just lower as the argument name. It's clearly understandable that lower in context of camelcase means exactly lower camelcase, which is a common term. No need for a long and explicit name here.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

lowercase_first_letter is too long.

I'd suggest one of the following (in order of preference):

  • downcase_first
  • lower
  • lower_first
@wontruefree

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

@straight-shoota @ysbaddaden what do you think about downcase as the argument?

src/string.cr Outdated Show resolved Hide resolved

@straight-shoota straight-shoota changed the title add lowercase option to String#camecase add lowercase option to String#camelcase Apr 25, 2019

@straight-shoota
Copy link
Member

left a comment

I'm okay with this, but I'd prefer to name the argument more explicitly downcase_first.

src/string.cr Outdated Show resolved Hide resolved
@vladfaust

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

That’s bad. camelcase(downcase: true) is confusing. camelcase(lower: true) is better. And I am for breaking change if needed to move the argument to the first place.

@j8r

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Or lower_first, that's about a bit longer than the current downcase and remove any ambiguity

@straight-shoota

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

And I am for breaking change if needed to move the argument to the first place.

No. It should only be usable as a named argument to clearly express the semantics.

Update src/string.cr
Co-Authored-By: wontruefree <jack@kilmer.co>
@wontruefree

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@straight-shoota CI seems to be failing due to running too long. Is this common for CI and how can I rerun it?

@wontruefree

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@straight-shoota thank you for re-running this.
Can we merge this?

@asterite asterite merged commit e398c4c into crystal-lang:master May 1, 2019

5 checks passed

ci/circleci: check_format Your tests passed on CircleCI!
Details
ci/circleci: test_darwin Your tests passed on CircleCI!
Details
ci/circleci: test_linux Your tests passed on CircleCI!
Details
ci/circleci: test_linux32 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asterite asterite added this to the 0.29.0 milestone May 1, 2019

@vladfaust

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

That's disturbing.

@wontruefree wontruefree deleted the wontruefree:camelcase branch May 1, 2019

@wontruefree

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

@vladfaust Which part is disturbing?

@ysbaddaden

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I'm late to the party, but where the hell did downcase came from? Downcase camelcase doesn't mean anything, it's just confusing.

This is lower or upper camelcase (which affects the first letter) not downcase —a completely different method and meaning 😕

@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 1, 2019

@ysbaddaden It downcases the first character, the equivalent method is #downcase not #lower, so it makes some sense. As mentioned, I'd have prefered downcase_first or lower_first.

@straight-shoota

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Hm, actually, it doesn't even downcase the first character when it's uppercase: "Foo".camelcase(downcase: true) => "Foo". I think this behaviour should be changed to return "foo".

@asterite

This comment has been minimized.

Copy link
Member

commented May 1, 2019

I think the API is fine, the above is just a bug in this PR's implementation.

@r00ster91

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Isn't this method generally behaving incorrectly? This method returns "EiffelTower" for "eiffel_tower" but that's wrong. "EiffelTower" is PascalCase, not camelCase. The first letter should be lowercase by default. I think there should be another method, pascalcase that has the current behavior of camelcase.

@j8r

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

@r00ster91 Pascal case was discussed in the related issue, please do not duplicate the discussion in this PR.

@bcardiff bcardiff changed the title add lowercase option to String#camelcase add downcase option to String#camelcase May 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.