Skip to content

#4788: remove System.getProperty in DefaultGenerator#5119

Closed
ePaul wants to merge 2 commits intoswagger-api:masterfrom
ePaul:4788-remove-System.getProperty-in-DefaultGenerator
Closed

#4788: remove System.getProperty in DefaultGenerator#5119
ePaul wants to merge 2 commits intoswagger-api:masterfrom
ePaul:4788-remove-System.getProperty-in-DefaultGenerator

Conversation

@ePaul
Copy link
Copy Markdown
Contributor

@ePaul ePaul commented Mar 19, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change.
    (I tried to run all ... there are many unrelated changes. So I first opened a bunch of PRs for just updating the stuff. More to come. When those are merged, I'll rebase this on master and run it again.) ❌ Please do not merge yet.
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes. → not sure, see below.

Description of the PR

(This is a part of a larger effort to get rid of the usage system properties for customizing the behavior of Swagger-Codegen, see #4788.)

Old state

DefaultGenerator was using System.getProperty for these purposes:

  • knowing if debug mode was requested (just the existence is enough)
    • debugSwagger
    • debugModels
    • debugOperations
    • debugSupportingFiles
  • knowing if the user wants to generate tests and/or documentation (these are (stringified) booleans, default true):
    • modelTests
    • apiTests
    • modelDocs
    • apiDocs
  • knowing if the user wants to generate just some of the models/APIs/supporting files (and which ones) – these are comma-separated lists.
    • supportingFiles
    • apis
    • models

Changes

We introduced a systemProperties property (a Map<String,String>) in ClientOptInput, which is used by DefaultGenerator instead of System.getProperty() now. I introduced some private convenience methods here to make the access easier for the three use cases we have.

CodegenConfigurator now puts its own systemProperties map (which comes from either the maven plugin or the CLI's -D options) into this field. (Currently, it also uses System.setProperty, I'll remove this in a later PR when all usages are removed.)

One test which used System.setProperties needed to be fixed.

Open issues

Samples: I did not update any samples. In theory, those should not be affected, but who knows if I missed anything. I tried to use bin/run_all_petstore, but there were many (mostly unrelated) changes. So I started to update samples based on master first (see e.g. #5118 and #5117, there will be some more.)

Breaking change? I'm not sure this counts as a breaking change. Strictly said, if one did give those properties as real system properties (e.g. in the CLI version with -DapiDocs=false before the class name instead of after it, or in maven from outside the project instead of in the <systemProperties> element), it won't work anymore. For the "usual" way of giving them (<systemProperties> or -D after the class name), it still works. Should this go into master, 2.3.0 or even a later version?
Should we introduce some compatibility layer, which fetches real system properties and collects them into the systemProperties map?

@ePaul
Copy link
Copy Markdown
Contributor Author

ePaul commented Mar 21, 2017

Fixed the tabs. (I guess I'll need to reconfigure my home IDE.)

@ePaul ePaul changed the title #4788: remove system.get property in default generator #4788: remove System.getProperty in DefaultGenerator Mar 27, 2017
Instead, we pass a systemProperties map in ClientOptInput.
@ePaul ePaul force-pushed the 4788-remove-System.getProperty-in-DefaultGenerator branch from dba48f7 to 5230f08 Compare March 27, 2017 20:03
@ePaul ePaul force-pushed the 4788-remove-System.getProperty-in-DefaultGenerator branch from 5230f08 to eb598e3 Compare March 27, 2017 20:28
@ePaul
Copy link
Copy Markdown
Contributor Author

ePaul commented Mar 27, 2017

This needs some reconsideration, I'll reopen it later.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants