-
Notifications
You must be signed in to change notification settings - Fork 6k
Make generated services conform to tslint rules set by angular-cli #7323
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
Conversation
There was a problem hiding this 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.
Thanks for the PR!
Good point, I agree.
I'll keep that in mind for the next lint change PR.
The fact that different people have preferences it valid; however, this library should be consistent with itself. As an example, as it is today, quotemarks are mixed so no matter if I have quotemarks set to "single" or "double" I will receive linting errors. Other rules like prefer-const are just considered best practice and, as far as I know, has no one opposing them. In the end, this library should have a style that it follows, and by far the most popular tslint rules for a typescript-angular project are the ones that come with angular-cli.
@manbearwiz You make a valid point in the generated code should be consistent with itself. I can agree with this being an effort to get the generated code conforming to best practice TS rules but if that is the case we should always in the future only approve pull requests that conform to those rules to stay consistent.
I'd be fine with approving this if that is the game plan but we'll have to be extremely vigilant in reviews from now on. At the very least we should add a test that runs TSLint with the said rules and does not accept a change if it doesn't conform to the proper coding rules.
@TiFu @sebastianhaas @Vrolijkx @taxpon thoughts?
I think there are certain advantages to enforcing a style guide. Like @manbearwiz said, certain best practices like prefer const are not controversial at all. If we want to enforce this, we definitely need to add tslint to the tests to make it easy to verify. It would also make reading the generated code a bit easier.
One issue with this approach is, that changes like max line length might not work for every swagger configuration. E.g. for api endpoint methods the line length might depend on the number of input parameters. This inconsistency could become a pain point, if people start to complain about their generated code not honoring tslint. Some other options like white-spacing rules would be useful, because we can then guarantee that those rules are also honored in the generated code.
What's your opinion on this?
We should also take into account that we have 3 (or more?) other TS clients. IMO we should use one style for all of them. I would propose to stick to one of the styles used by Microsoft e.g. TS Contrib or the config used for the TS compiler . Open for discussion though, I'd assume that most of the other styles (angular-cli, ...) are very similar.
@TiFu you make a good point about the max line length for function definitions. We could add something like // tslint:disable-next-line:max-line-length above these functions in the template to disable that rule for specific lines. Alternatively, we could put each input parameter on a new line but I think that would look worse.
As for their being many different tslint configs out there you're totally right.
- Microsoft/tslint-microsoft-contrib
- Microsoft/TypeScript
- angular/angular-cli
- angular-cli output
- angular/angular
- palantir/tslint
At first look, a lot of these seem to contradict; however, I believe we can satisfy the majority of them by going with the most strict version of a rule. For example, some want line lengths of 140 some want 100 but 100 satisfys both. Most use the no-console rules but some have different functions they allow; by disallowing all console functions you satisfy everyone. Because of this, I think it would make sense to start with a rule set and allow changes to it as long as it is more strict and does not contradict the rest of the rules.
itsTeknas
commented
Mar 16, 2018
Hello, I'm new here. I just imported the generated module in my project and saw the TSlint errors (loads of them). A Simple ng lint --fix fixed a lot of them directly. Even I was headed to make a contribution to fix these errors and then I stumbled upon this thread.
From a user perspective, when the editor.swagger.io websites lets you generate a client for typescript-angular shouldn't we safely assume that the user intends to import this module into an Angular project?
If so then we must strive to make the user experience better by conforming to the Default angular specific TSlint style guides. People are always allowed to have project-specific style guides. But the majority of developers will tend to stick to the style guides provided by the angular framework.
Just a suggestion.
EvAlex
commented
Nov 25, 2018
The discussion is misleading. Current PR should absolutely be merged. It makes generated code much better. There are some code style rules that are not subjective.
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.@TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx
Description of the PR
Make generated services conform to tslint rules set by angular-cli; particularly:
import-spacingmax-line-lengthno-trailing-whitespaceprefer-constquotemarksemicolon{ "import-spacing": true, "max-line-length": [ true, 140 ], "no-trailing-whitespace": true, "prefer-const": true, "quotemark": [ true, "single" ], "semicolon": [ true, "always" ] }