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

Fix Macro.camelize/1 for screaming snake case #5631

Closed
wants to merge 1 commit into from

Conversation

@devonestes
Copy link
Contributor

devonestes commented Jan 8, 2017

It was pointed out that Macro.underscore/1 had some issues dealing
with screaming snake case, and in the comments for that issue
@eksperimental also noted that Macro.camelize/1 was buggy with
screaming snake case as well. I've dealt with the Macro.underscore/1
bug in a separate commit, but this commit handles the secondary bug.

It was pointed out that `Macro.underscore/1` had some issues dealing
with screaming snake case, and in the comments for that issue
@eksperimental also noted that `Macro.camelize/1` was buggy with
screaming snake case as well. I've dealt with the `Macro.underscore/1`
bug in a separate commit, but this commit handles the secondary bug.
Copy link
Contributor

kelvinst left a comment

LGTM!

@adkron
Copy link
Contributor

adkron commented Jan 16, 2017

I just saw your PR. I diid #5662 on the exact same issue. I saw the comment in #5627, but since they weren't linked I went for it. We ended up with close to the same solution, but I was able to remove a few clauses. Should we merge the two?

@josevalim
Copy link
Member

josevalim commented Jan 18, 2017

Thank you! I have merged #5662 since it also includes a refactoring.
❤️ 💚 💙 💛 💜

@josevalim josevalim closed this Jan 18, 2017
@adkron
Copy link
Contributor

adkron commented Jan 18, 2017

@devonestes, I wrote an article about the refactoring in case you would like to take a look. I wanted to thank you for the code you put in so I made sure to do that in the article too. Have a good day.
http://dirtyinformation.com/blog/2017/01/18/two_duplication_refactorings_in_elixir

@devonestes
Copy link
Contributor Author

devonestes commented Jan 19, 2017

@adkron That was awful nice of you - thanks! And good refactoring, too!

@devonestes devonestes deleted the devonestes:Issue-5627-2 branch Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.