Skip to content

Correct some Java Warnings#1855

Closed
hiveship wants to merge 34 commits intoswagger-api:masterfrom
hiveship:master
Closed

Correct some Java Warnings#1855
hiveship wants to merge 34 commits intoswagger-api:masterfrom
hiveship:master

Conversation

@hiveship
Copy link
Copy Markdown
Contributor

@hiveship hiveship commented Jan 8, 2016

Just correct some Java warnings (static methods, useless imports, unused
parameters...). Don't impact any features. Tests are OK.

Just correct some Java warnings (static methods, useless imports, unused
parameters...). Don't impact any features. Tests are OK.
Tests OK.
*-SNAPSHOT are only available while using the local source. Refers to the latests on the Maven Central if using distant project.
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 12, 2016

Thanks for the PR. Instead of using tab, please use 4-space for indentation instead.

I notice that you convert many private instance method into static one. Is that a reason for making that change? Is that based on some Java best practices?

@hiveship
Copy link
Copy Markdown
Contributor Author

It's a Java Best practice that a method which do not need any instance variable may be declared as static. I only did that on private method since no one will be allow to override it in any other class.
If it make sense to call this method, even if no object has been constructed yet, then it should be static.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 12, 2016

Thanks. Do you have the URL to the section in best practice that we can find out more?

@hiveship
Copy link
Copy Markdown
Contributor Author

I don't have a link to the oracle website yet but:

See the accepted answer: http://stackoverflow.com/questions/11240178/what-is-the-gain-from-declaring-a-method-as-static

generated in Java using "java.util.UUID" class instead of a classical
String.
@hiveship
Copy link
Copy Markdown
Contributor Author

I added a commit to handle UUID generation in a Java code.

Shyri and others added 20 commits January 14, 2016 12:27
…resent. Clean responses mechanism using volley default's
This last change was producing a wrong swagger spec

@see https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md
"The value MUST start with a leading slash (/)"
Inspired by ClojureClientCodegen.java, it is interesting to use projectName from info.title swagger spec to fill up package.json name attribute
io.swagger.models.Info dependency added
projectName was already defined so never overriden.
… options

Adds option to use DateTimeOffset to model datetime fields instead of
DateTime to allow preservation of timezone information. Modifies
ApiClient.ParameterToString to support DateTimeOffset. Also adds
sourceFolder option.
A swagger file that contains a definition named "object" compiles into a
class named Object. This is technically not a problem as it lives in a
different package than java.lang.Object, but this requires the templates to
refer to Java's Object using its fully qualified name.
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 14, 2016

The change looks reasonable to me after reading the SO link you provided.

@fehguy do you have any concern switching private instance methods to private static methods?

@fehguy
Copy link
Copy Markdown
Contributor

fehguy commented Jan 14, 2016

Hi, I can't merge with tabs vs. spaces, it makes it very painful to diff and doesn't follow our coding standards.

The guts of the PR, from the comments, sound fine--but please try to send a PR with just your changes. If formatting is a problem, it's best to send formatting changes in a different PR.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 15, 2016

@hiveship seems like there's an issue with "rebase" as the PR contains 60+ commits. Let me know if you need help fixing that (e.g. using cherry-pick with a new branch)

@hiveship
Copy link
Copy Markdown
Contributor Author

@wing328 Yes, I would appreciate some help to fix that (I'm really new with git & GitHub).

@fehguy Do you have a code formatter (for example for Eclipse) to provide ? or any quick solution to fix my indent problems ?

Added basics files. Compiling, tests OK but not tested yet.
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 15, 2016

@hiveship please try the following:

git checkout master
git pull upstream master  // assuming upstream is a remote pointing to swagger-api/swagger-codegen.git
git checkout -b fix_java_warnings
git cherry-pick 7891873c6da49726e36159db597a39cf263dcde0

Check the files to confirm the change is present and cherry-pick more commits if needed.

# Conflicts:

#	modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/PerlClientCodegen.java
# Conflicts:

#	modules/swagger-codegen/src/main/java/io/swagger/codegen/DefaultCodegen.java

#	modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/JaxRSServerCodegen.java
@hiveship
Copy link
Copy Markdown
Contributor Author

@wing328 all failed and now my local project is in a very bad state (can not commit or pull anything). Is there is way to delete all my local project and reclone again ? (without loosing my work ?)

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 15, 2016

@hiveship no worry. Please don't delete any commit/branch.

I'll create a new branch and cherrypick the following commits this weekend when I've time:
7891873
f8f9066
b8df13a

@hiveship
Copy link
Copy Markdown
Contributor Author

Can close this PR. Too complicated to extract the commits and revert the original code formatting. I will continue my work on the Apache CXF support and I'll send a new pull request when it's finished. (See my question #1888)

@hiveship hiveship closed this Jan 15, 2016
@hiveship
Copy link
Copy Markdown
Contributor Author

@wing328 if you prefer, I can delete my fork and fork again. I can apply again my changes in a new branch and then ask for a pull request on this branch. I can do the same for my other works;

  • Support generation of Java UUID Type
  • Support for JAX-RS Apache CXF

@hiveship hiveship reopened this Jan 15, 2016
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jan 15, 2016

@hiveship well I just want to save you some time in manually applying those changes in a newly-forked repo. I'll leave it up to you on how you would like to proceed.

One suggestion is to create a new branch and avoid making change directly in the master.

@hiveship
Copy link
Copy Markdown
Contributor Author

Thank you but I think it could be better to apply change is a newly-forked repo since I am working on other features in the codegen, and the problem will be the same for next PR.
I will create separates branches !

@hiveship hiveship closed this Jan 15, 2016
@hiveship hiveship mentioned this pull request Jan 15, 2016
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.

10 participants