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

Should TSLint just be disabled for generated files? #13

Closed
jcloutz opened this issue Feb 7, 2018 · 5 comments
Closed

Should TSLint just be disabled for generated files? #13

jcloutz opened this issue Feb 7, 2018 · 5 comments

Comments

@jcloutz
Copy link

jcloutz commented Feb 7, 2018

Would it make sense to just disable to TSLint for the generated files? It doesn't effect functionality of the generated files, but it is annoying having the errors constantly pop up. It can also cause issues if TSLint is part of a CI process of pre-commit hook.

/* tslint:disable */
@agreatfool
Copy link
Owner

Sure.

I would check tomorrow if I could add some param in plugin execution. It's better to give the choice to users.

Like:

protoc \
--plugin=protoc-gen-ts=./bin/protoc-gen-ts \
--ts_out=no_lint=yes:${PROTO_DEST} \
-I ./proto \
proto/*.proto

And could you please give me some more information about the TSLint issues. Like the reproducing process.

@jcloutz
Copy link
Author

jcloutz commented Feb 7, 2018

Having more options is never bad. Maybe add an option for disabling it for the entire file, or pass a list of rules to disable? /* tslint:disable:rule1 rule2 rule3... */

Examples of the errors:

  • max-classes-per-file: unavoidable
  • member-access: Message objects have no member access modifiers
  • member ordering: static members come after non-static members and methods in generated messages
  • interface-over-type-literal: AsObject is a type instead of an interface, and it doesn't start with I if it was an interface

Non of the effects the fact that it just works though, it's really just aesthetics and personal code style preferences.

I'm just not sure I see the need to have linting in generated files, it either works or it doesn't. Some changes could be made to make them pass the default tslint rules for the most part, except for the multiple classes rule which can't be avoided. However, if someone is using a modified tslint config it may throw different errors.

If you want I can create a pull request to change the generated output so that it will pass the of the default tslint rules.

@agreatfool
Copy link
Owner

Finally I just disabled TSLint just like you mentioned at the beginning. New version released just now: v2.1.2.

Since I thought through again. Codes really running are the generated ones, from grpc-tools. And what current tool do is just add some definition for typescript.

So whatever we do it's just meaningless. Our goal is to make tsc understand the generated js codes, and it's already done. That's OK.

What do you think?

@jcloutz
Copy link
Author

jcloutz commented Feb 8, 2018

That make sense to me, there is no reason to lint something that shouldn't be changed :)

@agreatfool
Copy link
Owner

That's cool. Thank you for your issue & PR. :)

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

No branches or pull requests

2 participants