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

Remove linter option TSLint #5065

Merged
merged 9 commits into from Sep 10, 2020

Conversation

Shinigami92
Copy link
Contributor

@Shinigami92 Shinigami92 commented Jan 9, 2020

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Underlying tools
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

Other information:

References #5064

@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Jan 9, 2020

I don't know if removing the two tests is the right way, but in the past it seemed that they only passed if tsLint was set to true anyway?!

@sodatea
Copy link
Member

sodatea commented Jan 10, 2020

Thanks for the PR!
But FYI, it is a breaking change

@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Jan 10, 2020

Thanks for the PR!
But FYI, it is a breaking change

Thanks for your review
You are absolutely right, of course, and that was actually not intended at all.
This PR is only intended to remove the selection option and the associated code.
No code should be removed that has nothing to do with the generation and I probably removed a little too much here last night. I will revert this parts.

This PR should aim a cli version 4.2.0.
The complete removal of non-generating code should be done in another PR and could be in a cli version 5.0.

@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Jan 10, 2020

Ok ... since some tests use the generator to test linting with TSLint, I will completely revert the second commit

@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Jan 10, 2020

And because I just revert the changes and didn't use reset, we don't have to find the parts again for the upcoming breaking change PR :)

@sodatea sodatea changed the base branch from dev to next Sep 7, 2020
@sodatea
Copy link
Member

sodatea commented Sep 7, 2020

I just started working on the next major version.
I think we can revert the previous reversion, to introduce breaking changes :)

@Shinigami92 Shinigami92 force-pushed the remove-linter-option-tslint branch from 1bea82e to 4405277 Compare Sep 8, 2020
@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Sep 8, 2020

@sodatea Thanks for notifying me, I have updated this PR
Please have a look / review

@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Sep 8, 2020

The one test lint with no lintOnSave wants to have lintOnSave to be false
Maybe we should assign false as default value were it's defined? 🤔 (same for scripts.lint)

Or remove the expectations?

@sodatea
Copy link
Member

sodatea commented Sep 9, 2020

Yeah, I think we can just remove the two test cases

@Shinigami92 Shinigami92 requested a review from sodatea Sep 9, 2020
Copy link
Member

@sodatea sodatea left a comment

Thanks!

@sodatea sodatea merged commit adb8c7d into vuejs:next Sep 10, 2020
8 checks passed
@Shinigami92 Shinigami92 deleted the remove-linter-option-tslint branch Sep 10, 2020
@Shinigami92
Copy link
Contributor Author

Shinigami92 commented Sep 10, 2020

Thanks!

I'm happy to be part of the Vue ecosystem and community

I am happy to return your thanks to the community ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants