Make generated services conform to tslint rules set by angular-cli #7323
Conversation
|
I've discussed this before and this should not be merged. Different
projects have different rules and therefore we cannot as a library
generator confom to them all. @sebastianhaas agrees.
The idea for tslint is to make sure code conforms to rules so when
multiple people are writing it, it's all written the same way. In this
case it is generated so no one should be changing that code. Best
practice is to import it as a module external to the project.
What happens when someone else comes along and has max chars 80 instead of
the angular cli standard? If we merge stuff like this the changes would
never end of different people with different preferences. There is no hard
or even soft correct answer when it comes to these types of changes.
Edited: I wrote it on my phone and it had way too many spelling errors. my apologies!
…On Fri, Jan 5, 2018, 6:32 AM Tino Fuhrmann ***@***.***> wrote:
***@***.**** approved this pull request.
Looks good to me.
Thanks for the PR!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7323 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtcfvIMd8sdcY2VrubtWl-WZsK8yoks5tHggvgaJpZM4RUHSa>
.
|
|
Good point, I agree. |
|
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 |
|
@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. 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 As for their being many different tslint configs out there you're totally right.
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 |
|
Hello, I'm new here. I just imported the generated module in my project and saw the TSlint errors (loads of them). A Simple From a user perspective, when the editor.swagger.io websites lets you generate a client for 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. |
|
I'm not sure if this is possible (I don't know how angular works at all),
but ideally you want to add the generated module as a dependency only.
If that's what you did ignore this comment. Currently travelling and don't
have my laptop with me to look into this more.
It's quite hard to satisfy all rules reliably (e.g. line length) and as
others already mentioned different people might use different rule sets.
I also don't think it's worth the effort to conform strictly to arbitrary
rules in automatically generated code as long as the code is readable.
Which style errors did you encounter?
Am 16.03.2018 16:26 schrieb "Sanket Berde" <notifications@github.com>:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7323 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFs3Z53k3KV8HB7S_eWanMydNmbnDymzks5te3c4gaJpZM4RUHSa>
.
|
|
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" ] }