Skip to content

Improved CaseChangers #176

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

Closed
wants to merge 5 commits into from
Closed

Improved CaseChangers #176

wants to merge 5 commits into from

Conversation

oscargus
Copy link
Contributor

Added more words that should be kept lower case and added functionality to honor words within {}. Can readily be merged with 2.11 as well (for someone more handy with GitHub...).

Do not change words within {} (unfortunately still changes segments with spaces in).
Added more words that should be kept lower case in title case.
@matthiasgeiger
Copy link
Member

Thanks for your pull request!

One thing to discuss is the handling of curly braces...
Some examples:

  • This is a simple example {TITLE} should be This is a Simple Example {TITLE}
  • This {IS} another simple example tit{LE} should be This {IS} Another Simple Example Tit{LE}
  • {What ABOUT thIS} one? should be {What ABOUT thIS} one?
  • {And {thIS} might {a{lso}} be possible} should be {And {thIS} might {a{lso}} be possible}

Your solution only covers the first and parts of the second example.
I already racked my brains about this point yesterday and came up with no really satisfying solution. The only working implementation I could produce was to save all parts between the outermost curly braces (by counting braces 😳) and to restore the changes after performing the case changes on the input string...

Anybody else another idea?

@oscargus
Copy link
Contributor Author

You are correct.This limitation is mentioned (to some extent) in the commit message. The PR says improved, not perfect. :-) Third case should be manageable, the second and fourth are quite esoteric in my opinion.

@matthiasgeiger matthiasgeiger self-assigned this Sep 25, 2015
@matthiasgeiger
Copy link
Member

We decided in the team to support all of the four cases.

Do you want to implement the remaining logic?
Or shall we merge your PR into our code base and we will provide the remaining cases?

@simonharrer
Copy link
Contributor

At the moment, each implementation has to split the title and handle the logic to decide what is in {}brakets and what isnt. I would propose to extract that functionality.

I also vote for tests! :)

Fixed a bug which sometimes stopped the import
Replaced print outs with logging
Simplified some code to avoid multiple calls
@oscargus
Copy link
Contributor Author

oscargus commented Oct 1, 2015

Been busy with (real) work for some time...

I'm not really sure where the @Formatter:on/off should be. Can I see that somehow?

While I agree that it would be excellent to add handling of all four cases, I believe that the current approach is still a major leap (making it not break all formatting at least), so I'd say go ahead and merge. I will not work on it immediately, but given that no one else does it, I guess I may eventually get around to it.

@oscargus
Copy link
Contributor Author

oscargus commented Oct 1, 2015

Oh, now I also see that my next, independent, bug fix ended up in this pull request. This basically shows that I have quite limited knowledge of the GitHub and pull request system, unfortunately... Sorry about any mess caused. The idea was to create a separate pull request for the IEEEXploreFetcher bug fix (which I ran in to during the research application I was working on day and night for the last few days).

@oscargus
Copy link
Contributor Author

oscargus commented Oct 1, 2015

With that said: I clearly understand the idea with pull requests. I've just not been able to figure out how to work with them (or probably more GitHub) in practice, but this seems like a subject not to continue discussion here any further. I'll read up!

@matthiasgeiger
Copy link
Member

QuickTip: Create a branch for each PullRequest - all commits to this branch will end up in the same PR.

@oscargus oscargus closed this Oct 1, 2015
@oscargus
Copy link
Contributor Author

oscargus commented Oct 1, 2015

I'll start all over and provide nice clean PRs from now on... ;-)

@igorsteinmacher
Copy link
Member

Great @oscargus :-)

@koppor
Copy link
Member

koppor commented Oct 6, 2015

New PR is #192

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.

5 participants