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

Update --skip-validate-spec option with "--validate-spec" #740

Closed
wants to merge 1 commit into from

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Aug 5, 2018

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, 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

Update --skip-validate-spec option with "--validate-spec" to make consistent with the maven/gradle plug-in option. An improvement to #251

@jimschubert please review to see if this change looks good to you.

@wing328 wing328 added this to the 3.2.0 milestone Aug 5, 2018
@wing328
Copy link
Member Author

wing328 commented Aug 5, 2018

The test failure:

testTypeMappings(org.openapitools.codegen.cmd.GenerateTest)  Time elapsed: 0.004 sec  <<< FAILURE!
mockit.internal.UnexpectedInvocation:
Unexpected invocation of:
org.openapitools.codegen.config.CodegenConfigurator#setValidateSpec(boolean)
   with arguments: true
   on mock instance: org.openapitools.codegen.config.CodegenConfigurator@57c758ac
	at org.openapitools.codegen.cmd.GenerateTest.testTypeMappings(GenerateTest.java:329)
Caused by: mockit.internal.expectations.invocation.ExpectationError
	at org.openapitools.codegen.cmd.GenerateTest.setupAndRunTest(GenerateTest.java:563)
	at org.openapitools.codegen.cmd.GenerateTest.setupAndRunGenericTest(GenerateTest.java:577)
	at org.openapitools.codegen.cmd.GenerateTest.testTypeMappings(GenerateTest.java:327)

@jimschubert let me know if my approach (workaround) is ok and I'll fix the test accordingly

@wing328 wing328 modified the milestones: 3.2.0, 3.2.1 Aug 6, 2018
@jimschubert
Copy link
Member

After some discussion, I'm going to close this for a handful of reasons which I'll enumerate here.

  1. CLI and Maven/Gradle plugins have slightly different use cases. Although all three end up in code being generated, the CLI is meant to be executed directly by the user or user scripts while the plugins are meant to be declaratively configured and executed by a build tool. As such, CLI options are user-focused while plugin options are value-focused.

  2. Changing to an --option=string option rather than a --switch option would make this inconsistent with how other switches are applied at the CLI level.

  3. The current switch --skip-validate-spec follows common Linux/Unix command format for switches, for example from man git-commit:

           -n, --no-verify
             This option bypasses the pre-commit and commit-msg hooks. See also 
    githooks(5).
    
           --allow-empty
               Usually recording a commit that has the exact same tree as its sole 
    parent commit is a mistake, and the command prevents you from making such a
               commit. This option bypasses the safety, and is primarily for use by 
    foreign SCM interface scripts.
    

    In the above example, git allows skipping with the more common --no- prefix. I chose --skip- because you can find this in some CLI tools, and we have a large non-English speaking user base; I felt that --skip- would translate more easily than simply --no-. I could see us using the same option names as above for opt-out and opt-in behaviors.

  4. Changing from a switch to a string-valued option doesn't add any apparent value. I could see someone who is intimately familiar with options in the plugin trying to apply those same options to the CLI tool, but they'd immediately be presented with an error and then run cli help and find the correct option. I've contributed to the codebase for over 2 years, and I don't know all options well enough to pass them effortlessly between the plugins and the CLI… so I think the likelihood of this is pretty low. More commonly, I'd assume people work similarly to how I work (open plugin README for options, execute cli help or cli config-help for options).

We could definitely change the command line option name if the community thinks it's necessary. We could also include both options (--skip-validate-spec as boolean and --validate-spec=[true|false] as string option). Changing the existing option name for a new option name would be a minor breaking change for the CLI.

@jimschubert jimschubert closed this Aug 6, 2018
@jimschubert jimschubert deleted the update_option branch August 6, 2018 13:28
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.

2 participants