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

Decamelize #472

Merged
merged 6 commits into from
Sep 17, 2019
Merged

Decamelize #472

merged 6 commits into from
Sep 17, 2019

Conversation

dolezel
Copy link
Contributor

@dolezel dolezel commented Aug 13, 2019

Fixes #464
It has great potential to break something, so I'm not sure I want to merge it :/

@dolezel dolezel requested a review from roman-kaspar August 13, 2019 12:26
@roman-kaspar
Copy link

Files changed: 124
Diff: +2,002; -1,669

I am not reviewing this PR as is.

There is a ton of totally unrelated changes.
E.g. caused by this: singleQuote: true,

So if you want me to review it, please split the PR to parts relevant to the Decamelize topic (in dedicated PR to review), and the rest (that can be probably merged without review (guessing the changes are generated by some linting tool anyway)).

@dolezel dolezel force-pushed the decamelize branch 5 times, most recently from bcc0a99 to 4f2a792 Compare August 14, 2019 07:24
@dolezel
Copy link
Contributor Author

dolezel commented Aug 14, 2019

I've split it with commits. Only relevant to decamelize is 4f2a792
It is still big, but I have no idea how to split it more...

@roman-kaspar
Copy link

It is still big, but at least the half of the changes are gone.
Looking at it now.

Copy link

@roman-kaspar roman-kaspar left a comment

Choose a reason for hiding this comment

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

Did not verify all the details that the text-generating functions still do what they did before (hope tests will cover that).

But I don't like this PR (yet). See the two comments.

Copy link

@roman-kaspar roman-kaspar left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments :-).
LGTM.

@dolezel dolezel merged commit e750092 into master Sep 17, 2019
@dolezel dolezel deleted the decamelize branch September 17, 2019 08:18
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.

snake <> camel case conversion
2 participants