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

Delete --lang option #1592

Merged
merged 8 commits into from
Dec 9, 2018
Merged

Conversation

ackintosh
Copy link
Contributor

@ackintosh ackintosh commented Dec 1, 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, 3.4.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

Related issue: #39

Delete --lang option which is deprecated.

@ackintosh ackintosh changed the base branch from 4.0.x to master December 2, 2018 08:10
@ackintosh
Copy link
Contributor Author

I've rebased and changed the base branch to master according to #1594.

@jmini
Copy link
Member

jmini commented Dec 5, 2018

continuous-integration/appveyor/pr — AppVeyor build failed

According to AppVeyor build there are still Windows script that are using -l.


Maybe we should start a new section in migration-guide.adoc (or a new document) with all important changes made in 4.0.0.

Removing a flag (even if it was deprecacted for a year) is an important change.

@ackintosh
Copy link
Contributor Author

Hmm... I can't find the script contains -l.

$ git grep ' -l typescript-angular'
$

@ackintosh
Copy link
Contributor Author

cc @OpenAPITools/generator-core-team

@wing328
Copy link
Member

wing328 commented Dec 8, 2018

Hmm... I can't find the script contains -l.

➜  openapi-generator git:(ackintosh-delete-lang-option) grep -R " \-l " bin/windows/*
bin/windows/typescript-angular-v7-not-provided-in-root-with-npm.bat:set ags=generate -i modules\openapi-generator\src\test\resources\2_0\petstore.yaml -l typescript-angular -c bin\typescript-angular-v7-petstore-not-provided-in-root-with-npm.json -o samples\client\petstore\typescript-angular-v7-not-provided-in-root\builds\with-npm -D providedInRoot=false --additional-properties ngVersion=7.0.0
bin/windows/typescript-angular-v7-not-provided-in-root.bat:set ags=generate -i modules\openapi-generator\src\test\resources\2_0\petstore.yaml -l typescript-angular -o samples\client\petstore\typescript-angular-v7-not-provided-in-root\builds\default -D providedInRoot=false --additional-properties ngVersion=7.0.0
bin/windows/typescript-angular-v7-provided-in-root-with-npm.bat:set ags=generate -i modules\openapi-generator\src\test\resources\2_0\petstore.yaml -l typescript-angular -c bin/typescript-angular-v7-petstore-provided-in-root-with-npm.json -o samples\client\petstore\typescript-angular-v7-provided-in-root\builds\with-npm --additional-properties ngVersion=7.0.0
bin/windows/typescript-angular-v7-provided-in-root.bat:set ags=generate -i modules\openapi-generator\src\test\resources\2_0\petstore.yaml -l typescript-angular -o samples\client\petstore\typescript-angular-v7-provided-in-root\builds\default --additional-properties ngVersion=7.0.0

I'm able to find a few -l used in the batch files (TS angular v7)

@jimschubert
Copy link
Member

@ackintosh I see two references to -l:

$ git grep ' -l '
bin/security/perl-petstore.sh:ags="generate -i modules/openapi-generator/src/test/resources/2_0/petstore-security-test.yaml  -l perl -o samples/client/petstore-security-test/perl $@"
scripts/openapi-generator-cli-completion.bash:        # openapi-generator-cli config-help -l YOUR_LANG | grep '^\t' | grep -v '^\t\s\s\s\s' | tr -d '\t'

First one should also cause the integration tests to fail, the second one is just a comment.

There are other instances of the -l option as well:

double quoted:

$ git grep '"-l"'
modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/ConfigHelp.java:    @Option(name = {"-l", "--lang"}, title = "language",

single quoted:

$ git grep "'-l'"

modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/ConfigHelp.java:                LOGGER.warn("The '--lang' and '-l' are deprecated and may reference language names only in the next major release (4.0). Please use --generator-name /-g instead.");

I also couldn't find reference to typescript-angular, but I don't have access to the appveyor job to see if it's potentially just cached/renamed scripts that aren't cleaned on each build.

So it looks like:

  • bin/security/perl-petstore.sh need to be changed to -g
  • lang option in ConfigHelp.java needs to be removed and the conditional log needs to be updated

@jimschubert
Copy link
Member

@wing328 that's weird. I run the same grep and I get no results:

jim at engineer in ~/projects/openapi-generator on ackintosh-delete-lang-option* 344c7fa80b
$ grep -R " \-l " bin/windows/*
jim at engineer in ~/projects/openapi-generator on ackintosh-delete-lang-option* 344c7fa80b

What grep are you using? Seems the grep Akihito and I are using might be broken.

$ grep --version
grep (BSD grep) 2.5.1-FreeBSD

@wing328
Copy link
Member

wing328 commented Dec 8, 2018

➜  openapi-generator git:(ackintosh-delete-lang-option) grep --version
grep (BSD grep) 2.5.1-FreeBSD
➜  openapi-generator git:(ackintosh-delete-lang-option) cat bin/windows/typescript-angular-v7-not-provided-in-root-with-npm.bat
set executable=.\modules\openapi-generator-cli\target\openapi-generator-cli.jar

If Not Exist %executable% (
  mvn clean package
)

REM set JAVA_OPTS=%JAVA_OPTS% -Xmx1024M
set ags=generate -i modules\openapi-generator\src\test\resources\2_0\petstore.yaml -l typescript-angular -c bin\typescript-angular-v7-petstore-not-provided-in-root-with-npm.json -o samples\client\petstore\typescript-angular-v7-not-provided-in-root\builds\with-npm -D providedInRoot=false --additional-properties ngVersion=7.0.0

java %JAVA_OPTS% -jar %executable% %ags%

If you read/edit bin/windows/typescript-angular-v7-not-provided-in-root-with-npm.bat directly, do you see the -l at line 8?

@wing328
Copy link
Member

wing328 commented Dec 8, 2018

Here is the error log (partial) in AppVeyor:

[main] INFO  o.o.codegen.AbstractGenerator - writing file C:\projects\openapi-generator\samples\client\petstore\typescript-angular-v6-not-provided-in-root\builds\with-npm\typings.json
[main] INFO  o.o.codegen.AbstractGenerator - writing file C:\projects\openapi-generator\samples\client\petstore\typescript-angular-v6-not-provided-in-root\builds\with-npm\tsconfig.json
[main] INFO  o.o.codegen.AbstractGenerator - writing file C:\projects\openapi-generator\samples\client\petstore\typescript-angular-v6-not-provided-in-root\builds\with-npm\.openapi-generator\VERSION
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0
[error] Found unexpected parameters: [-l, typescript-angular]
See 'openapi-generator-cli help' for usage.
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0
[error] Found unexpected parameters: [-l, typescript-angular]
See 'openapi-generator-cli help' for usage.
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0
[error] Found unexpected parameters: [-l, typescript-angular]
See 'openapi-generator-cli help' for usage.
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0
[error] Found unexpected parameters: [-l, typescript-angular]
See 'openapi-generator-cli help' for usage.
Java HotSpot(TM) 64-Bit Server VM warning: ignoring option MaxPermSize=2g; support was removed in 8.0

@jimschubert
Copy link
Member

@wing328 I don't seem to have the v7 scripts locally on this branch:

jim at engineer in ~/projects/openapi-generator on ackintosh-delete-lang-option* 344c7fa80b
$ ls -la bin/windows/typescript-angular-v7*
ls: cannot access 'bin/windows/typescript-angular-v7*': No such file or directory

Verified with ripgrep to rule out a grep version issue:

jim at engineer in ~/projects/openapi-generator on ackintosh-delete-lang-option* 344c7fa80b
$ rg ' -l ' .
./bin/run-all-petstore
13:for SCRIPT in $(ls -l ./bin/*.sh | grep -v all)

./scripts/openapi-generator-cli-completion.bash
29:        # openapi-generator-cli config-help -l YOUR_LANG | grep '^\t' | grep -v '^\t\s\s\s\s' | tr -d '\t'

./bin/openapi3/run-all-petstore
13:for SCRIPT in $(ls -l ./bin/openapi3/*.sh | grep -v all)

./bin/tests/run-all-test
13:for SCRIPT in $(ls -l ./bin/tests/*.sh | grep -v all)

./bin/security/perl-petstore.sh
31:ags="generate -i modules/openapi-generator/src/test/resources/2_0/petstore-security-test.yaml  -l perl -o samples/client/petstore-security-test/perl $@"

./samples/server/petstore/php-slim/php_syntax_checker.bash
5:    result=`php -l $i | grep "No syntax errors detected"`

./samples/client/petstore/cpp-tizen/doc/Doxyfile
666:# that represents doxygen's defaults, run doxygen with the -l option. You can

./modules/openapi-generator/src/main/resources/cpp-tizen-client/Doxyfile.mustache
666:# that represents doxygen's defaults, run doxygen with the -l option. You can

Looking at the directory structure on this branch, those files don't exist: https://github.com/OpenAPITools/openapi-generator/tree/344c7fa80bf715755240a2fde897bb144e7f6e44/bin/windows

Seems like you may have stale files locally, and those also exist on Appveyor?

@wing328
Copy link
Member

wing328 commented Dec 8, 2018

I can find it in the latest master: https://github.com/OpenAPITools/openapi-generator/blob/master/bin/windows/typescript-angular-v7-not-provided-in-root-with-npm.bat#L8 (added 3 days ago)

Looks like @ackintosh needs to merge the latest master into the branch and update those batch files accordingly.

@jimschubert
Copy link
Member

@wing328 I agree a rebase is in order.

Does appveyor automatically do this? I didn't see in appveyor.yml where this would occur, and I don't have access to see the job's configuration.

@wing328
Copy link
Member

wing328 commented Dec 8, 2018

I think appveyor automatically performs the merge into master so as Shippable as well (which tests the branch as well as the merge with the latest master - which means 2 shippable jobs for each change)

@ackintosh
Copy link
Contributor Author

Thanks for the investigation! I understand that I need to merge latest master into this branch.💡

@ackintosh
Copy link
Contributor Author

The tests has passed. ✨

@ackintosh ackintosh merged commit f197944 into OpenAPITools:master Dec 9, 2018
@ackintosh ackintosh deleted the delete-lang-option branch December 9, 2018 10:26
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
* Delete --lang option

* Fix -l

* Fix -l flag (windows)

* Add migration guide

* Change to -g

* Remove lang option

* Change to -g
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.

4 participants