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

Handle punctuation #6

Merged
merged 1 commit into from
Jan 14, 2018
Merged

Handle punctuation #6

merged 1 commit into from
Jan 14, 2018

Conversation

jhchen
Copy link
Contributor

@jhchen jhchen commented Jan 9, 2018

Following up on #5 with something to take a look at. If the approach is acceptable perhaps it would be better to move to snake_case.ex instead since it seems like the other cases depend on that one (and update the other tests).

I tried running mix credo --strict and mix dialyzer but they errorred on the main sobolevn/recase master branch as well so I don't think this PR introduces any new cod equality issues. If you're not seeing the same I can provide more details on what I'm seeing.

Last thing (and perhaps this should be separate) but using Lodash's implementation as a guide, they also split up strings with digits ex you can try _.snakeCase("ab-cD-e0-Df-GH-I1-2j-3K-45") === "ab_c_d_e_0_df_gh_i_1_2_j_3_k_45" in the developer console in lodash.com.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a930f6c on jhchen:master into 9185ac6 on sobolevn:master.

@sobolevn
Copy link
Member

@jhchen Thanks for your work! I appreciate it.

What about snake_case? Is it any different from kebab?
If it should behave in the same manner - it means, that we should fix it as well (or fix just snake_case since many other cases rely on it).

I think we should discuss lodash way of handling kebab case as a separate issue. Will you, please, open it?

@sobolevn sobolevn mentioned this pull request Jan 13, 2018
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f9edaac on jhchen:master into 9185ac6 on sobolevn:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f9edaac on jhchen:master into 9185ac6 on sobolevn:master.

@jhchen
Copy link
Contributor Author

jhchen commented Jan 14, 2018

Okay I updated the code and PR to implement the punctuation fix in snake case instead, updated the affected tests and opened a new issue for the digits question.

@sobolevn sobolevn merged commit 0416705 into wemake-services:master Jan 14, 2018
@sobolevn
Copy link
Member

@jhchen you are awesome!

@jhchen
Copy link
Contributor Author

jhchen commented Jan 18, 2018

Thanks! You have any thoughts on when to release a new version?

@sobolevn
Copy link
Member

I am actually releasing it in a few hours.
Right now I am trying to fix some minor issues with the CI and stuff.

@sobolevn
Copy link
Member

@jhchen done.

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

Successfully merging this pull request may close these issues.

3 participants