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.

Sign up for GitHub

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

Jump to bottom

Refactor StringUtils functions #859

Merged
merged 10 commits into from
Aug 30, 2018
Merged

Refactor StringUtils functions #859

merged 10 commits into from
Aug 30, 2018

Conversation

rienafairefr
Copy link
Contributor

@rienafairefr rienafairefr commented Aug 21, 2018 •
edited
Loading

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

As part of "Separation of Concerns" project and #842 I felt that the place for camelize, underscore, or dashize are not in the DefaultCodegen, they should be moved to a class in org.openapitools.codegen.utils, and then each language would have its own use for these static functions

I also extracted the escapeSpecialCharacters method of DefaultCodegen to a static method escape in StringUtils, looks better to me but that's not a mandatory part IMO

@jmini
Copy link
Member

jmini commented Aug 22, 2018

Can you please merge master into this PR?

@rienafairefr
Copy link
Contributor Author

Done 👍

Merge branch 'master'
5ebf1ac 
Conflicts:
	modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractEiffelCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractGoCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractPhpCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CSharpClientCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CSharpNancyFXServerCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/CppPistacheServerCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/HaskellHttpClientCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/LuaClientCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ObjcClientCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonFlaskConnexionServerCodegen.java
	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/RubyClientCodegen.java
wing328
wing328 approved these changes Aug 26, 2018
View reviewed changes
Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for enhancement 👍

@wing328 wing328 added this to the 3.2.3 milestone Aug 26, 2018
@wing328
Copy link
Member

wing328 commented Aug 27, 2018 •
edited
Loading

I tried to use the web-based interface to resolve the merge conflicts but failed. I'm working on a fix by pulling the branch locally.

UPDATE: pushed the fix via 20bd4d1

@wing328
Copy link
Member

wing328 commented Aug 27, 2018

@rienafairefr can you please take another look to ensure my change didn't mess up your PR? Sorry for the inconvenience.

@rienafairefr
Copy link
Contributor Author

rienafairefr commented Aug 27, 2018 via email

@rienafairefr
Copy link
Contributor Author

Your merge looks fine 👍
Re-looking at it, because this removes some public methods in DefaultCodegen this is a breaking change, shouldn't this be targeted to 4.0 ? I could easily reinstate the public methode, @Deprecate them while delegating to StringUtils inside each, then that wouldn't break any public thing, and could go in 3.2.3 . Your call, I guess

@wing328
Copy link
Member

wing328 commented Aug 27, 2018 •
edited
Loading

@deprecate them while delegating to StringUtils inside each, then that wouldn't break any public thing, and could go in 3.2.3.

👍 I think that's more preferable by using @deprecate to mark those methods to be deprecated later.

@rienafairefr
Copy link
Contributor Author

Done, they're deprecated now. A bit ugly to have to call with the fully qualified org.openapitools.codegen.utils.StringUtils.* name (as long as the methods are not removed, the names clash), but it seems to work. Might be cleaner to rename the methods.

put string utils methods as deprecated, call with FQDN to call the or…
0e0f77e 
…g.openapitools.codegen.utils.StringUtils static methods
@wing328
Copy link
Member

wing328 commented Aug 28, 2018

If no one has further feedback, I'll merge it tomorrow.

@wing328 wing328 removed this from the 3.2.3 milestone Aug 30, 2018
@wing328 wing328 added this to the 3.3.0 milestone Aug 30, 2018
@wing328 wing328 merged commit d49fb1c into OpenAPITools:master Aug 30, 2018
@wing328 wing328 changed the title StringUtils functions Refactor StringUtils functions Aug 30, 2018
@wing328 wing328 mentioned this pull request Sep 13, 2018
Merged
4 tasks
@wing328
Copy link
Member

wing328 commented Sep 13, 2018

@rienafairefr thanks again for the refactoring. I've filed a follow-up PR to remove those deprecated methods in 3.3.0 (minor release): #1031

@rienafairefr rienafairefr deleted the stringutils branch February 9, 2019 23:15
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@wing328
StringUtils functions (OpenAPITools#859)
88995ef 
* extract StringUtils

* extract escape function

* fixup! extract escape function

* forbiddenapis fix

* fix merge issue

* put string utils methods as deprecated, call with FQDN to call the org.openapitools.codegen.utils.StringUtils static methods

* javadoc fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 approved these changes

@jmini jmini jmini approved these changes

Assignees
No one assigned
Labels
Enhancement: Feature Feature: Generator
Projects
None yet
Milestone
3.3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rienafairefr @jmini @wing328