Skip to content

build: add tslint formatting task #4109

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

Merged
merged 1 commit into from
Apr 18, 2017
Merged

Conversation

crisbeto
Copy link
Member

Adds a Gulp task that fixes TSLint errors automatically when possible. This should make it easier to add/remove linting rules.

@crisbeto crisbeto requested a review from jelbourn April 15, 2017 14:28
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 15, 2017
@devversion
Copy link
Member

I don't really think that it is necessary to have such a task. It happens to rarely that we add/remove tslint rules.

Most of the time those rules can be fixed manually very quickly and if really necessary we can just run $(npm bin)/tslint **/*.ts --fix.

@crisbeto
Copy link
Member Author

I think it's fine, because:

  1. It's a simple one-liner.
  2. It's a lot easier to remember.
  3. It can still save us a lot of manual work.

@devversion
Copy link
Member

Hm I personally not agree with that.

  1. It doesn't really matter. It's about unnecessary stuff inside of our already big build process.
  2. That probably depends. tslint **/*.ts --fix feels more convenient to me (also is more flexible)
  3. As said we don't change rules very often and if we have to (rarely) then we can just run the command.

Decision might be up to @jelbourn. I just want to make sure that we don't add stuff to the build process that will be likely used once in a year. It should be as tight & clean as possible.

@crisbeto
Copy link
Member Author

FWIW, it's not only down to tslint **/*.ts --fix, you also need to pass it the path to a tsconfig.json.

@devversion
Copy link
Member

@crisbeto The tsconfig file needs to passed if the project uses rules that require TypeScript's TypeChecker from the program.

@crisbeto
Copy link
Member Author

My guess is that will be the case once we disable the custom rule for unused imports, which throws an error atm, and we start using tslint's built-in rule.

@devversion
Copy link
Member

Yeah but that is a separate issue to fix the getHighlights error from the language service. And yeah this rule would require the type checker.

@jelbourn
Copy link
Member

I'm all for auto-fix, and we should probably just make it a pre-commit hook for everyone (probably without gulp there).

@crisbeto what is the set of things it will fix? Presumably obvious things like missing semi-colon and using the wrong quotes, but I'd guess line-breaking would be out of scope.

@crisbeto
Copy link
Member Author

crisbeto commented Apr 17, 2017

I believe that it depends on whether the particular rule has a corresponding "fixer". Not sure if there's a good way to see which ones do, though. These are the built-in ones that do.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Apr 17, 2017
@jelbourn
Copy link
Member

@crisbeto just needs rebase

Adds a Gulp task that fixes TSLint errors automatically when possible. This should make it easier to add/remove linting rules.
@kara kara merged commit 4bfd2a3 into angular:master Apr 18, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants