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

Typescript Angular 2: Make service suffix configurable #341

Merged
merged 5 commits into from
Jun 25, 2018
Merged

Typescript Angular 2: Make service suffix configurable #341

merged 5 commits into from
Jun 25, 2018

Conversation

tomvangreen
Copy link
Contributor

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.1.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

In earlier versions of the typescript-angular (2) swagger-codegen / openapi-generator, the suffix of the generated api servies was Api. This has been changed to Service in order to conform to angular naming guidelines.

For our project it makes sense that we can chose a different prefix in order to avoid naming collisions. For this I introduced 2 configuration parameters. serviceSuffix and serviceFileSuffix.

Service suffix is used as a suffix in the generated class name. Service file suffix is used as suffix in the file name (before the .ts extensions).

Technical comitee typescript: @TiFu, @taxpon, @sebastianhaas, @kenisteward, @Vrolijkx, @macjohnny
#234

@kenisteward
Copy link
Contributor

kenisteward commented Jun 18, 2018 via email

@tomvangreen
Copy link
Contributor Author

tomvangreen commented Jun 18, 2018 •
edited

We are doing aliasing now, but we have a lot of services that exist in the api and have a counter part that we have written ourselves, which uses the same name (I know, this is not optimal, but we inherited this code base). Aliasing comes with some disadvantages. Auto import in IDE's does not work so well, as you anyway need to manually alias it after the import. Additionally we can never be sure by just looking at the class, if it is now a service or an api, because it might be that somebody forgot to alias the service.

Additionally, I really like to have the distinction between generated and self written serviecs. Having this api suffix makes it clear (without having to consult the import statement) on what exactly we are looking at. Honestly I was a bit bummed when the suffix has been changed from api to service.

All in all, it would make our lifes a bit easier and let us concentrate on the real work ;)

macjohnny
macjohnny approved these changes Jun 18, 2018
View reviewed changes
Copy link
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

looks good to me, although I haven't tested it

@kenisteward
Copy link
Contributor

kenisteward commented Jun 18, 2018 via email

@kenisteward
Copy link
Contributor

@tomvangreen did you regenerate the samples and got no changes?

@tomvangreen
Copy link
Contributor Author

tomvangreen commented Jun 18, 2018 •
edited

You're welcome for the pull request, wasn't too much work.

I would be happy if you could check that I did not break anything. I tried to implement the change in a non breaking manner (meaning you should have an identical output when you don't supply the new config parameters), but I would feel better if you could validate that before merging back.

Edit: I regeneretad them, but I made a small change since then. Let me try again to make sure.

@tomvangreen
Copy link
Contributor Author

I reran the angular tests (on windows with the script typescript-angular-petstore-all.bat ) and the only thing that has changed is the VERSION file in the .openapi-generator folder.

@kenisteward
Copy link
Contributor

kenisteward commented Jun 18, 2018 via email

@macjohnny
Copy link
Member

@tomvangreen could you please merge the most recent master into your branch to re-trigger the CircleCI tests?

@tomvangreen
Copy link
Contributor Author

@macjohnny : I did that but now the shippable build is failing. I looked at the logs and it looks like it is unrelated to my changes.

===> Fetching jesse ({git,"https://github.com/for-GET/jesse.git",
                          {tag,"1.4.0"}})
===> Fetching jsx ({git,"https://github.com/talentdeficit/jsx.git",
                        {tag,"2.8.2"}})
===> Skipping jsx (from {git,"git://github.com/talentdeficit/jsx.git",
                             {tag,"2.8.0"}}) as an app of the same name has already been fetched
===> Compiling jsx
===> Compiling jesse
===> Compiling _build/default/lib/jesse/src/jesse_tests_util.erl failed
_build/default/lib/jesse/src/jesse_tests_util.erl:92: erlang:get_stacktrace/0: deprecated; use the new try/catch syntax for retrieving the stack backtrace

[ERROR] Command execution failed.
org.apache.commons.exec.ExecuteException: Process exited with an error: 1 (Exit value: 1)
    at org.apache.commons.exec.DefaultExecutor.executeInternal (DefaultExecutor.java:404)
    at org.apache.commons.exec.DefaultExecutor.execute (DefaultExecutor.java:166)
    at org.codehaus.mojo.exec.ExecMojo.executeCommandLine (ExecMojo.java:804)
    at org.codehaus.mojo.exec.ExecMojo.executeCommandLine (ExecMojo.java:751)
...

@macjohnny
Copy link
Member

...it looks like it is unrelated to my changes.

@tomvangreen absolutely, it usually is due to some rate-limits for artifact downloads from other locations. Usually re-triggering the CircleCI build solves the issue. You can also manually do that by closing and re-opening your PR

@tomvangreen tomvangreen reopened this Jun 21, 2018
@macjohnny
Copy link
Member

@tomvangreen now it was the Shippable build that ran into the same issue...

@tomvangreen
Copy link
Contributor Author

@macjohnny What can I do about that? Should I wait a day and close and reopen again?

@macjohnny
Copy link
Member

@tomvangreen yes, so far I don't know of a better solution

@macjohnny
Copy link
Member

other PRs face the same problem, see e.g. #289 (comment)

@tomvangreen tomvangreen reopened this Jun 22, 2018
@wing328
Copy link
Member

wing328 commented Jun 25, 2018

The rust-server can be ignored (commented out in the master for the time being):

   Compiling petstore_api v1.0.0 (file:///home/travis/build/OpenAPITools/openapi-generator/samples/server/petstore/rust-server)
error: lint unused_doc_comment has been renamed to unused_doc_comments
 --> examples/server_lib/mod.rs:6:5
  |
6 |     error_chain!{}
  |     ^^^^^^^^^^^^^^
  |
  = note: `-D renamed-and-removed-lints` implied by `-D warnings`
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: lint unused_doc_comment has been renamed to unused_doc_comments
 --> examples/server_lib/mod.rs:6:5
  |
6 |     error_chain!{}
  |     ^^^^^^^^^^^^^^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: lint unused_doc_comment has been renamed to unused_doc_comments
 --> examples/server_lib/mod.rs:6:5
  |
6 |     error_chain!{}
  |     ^^^^^^^^^^^^^^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: lint unused_doc_comment has been renamed to unused_doc_comments
 --> examples/server_lib/mod.rs:6:5
  |
6 |     error_chain!{}
  |     ^^^^^^^^^^^^^^
  |
  = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to 4 previous errors

error: Could not compile `petstore_api`.
warning: build failed, waiting for other jobs to finish...

@wing328 wing328 added this to the 3.0.3 milestone Jun 25, 2018
@wing328 wing328 merged commit 38bb732 into OpenAPITools:master Jun 25, 2018
@wing328
Copy link
Member

wing328 commented Jun 25, 2018

@kenisteward @macjohnny thanks for reviewing the change.

@tomvangreen thanks for the PR. For your upcoming PRs, I would suggest creating a branch as per git best practices.

@tomvangreen
Copy link
Contributor Author

@wing328 You're welcome, in the end the change should make my live easier in multiple projects, so I'm happy to help. I messed up with the branch. I did the change first for swagger codegen and then I wanted to port the changes to openapi codegen and forgot to create the branch in this repo. I try to keep that in mind for future pull requests.

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

@macjohnny macjohnny macjohnny approved these changes

Assignees
No one assigned
Labels
Client: TypeScript Enhancement: Feature
Projects
None yet
Milestone
3.0.3
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tomvangreen @kenisteward @macjohnny @wing328