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

[Feature][#67] Adds tslint autofix as a flag to ForkTsCheckerWebpackPlugin plugin #174

Merged

Conversation

ajainarayanan
Copy link
Contributor

  • Adds tslintAutoFix as a flag to ForkTsCheckerWebpackPlugin plugin in webpack config.
  • Set the default to false for backward compatibility.
  • Updates Vue integration test based on changes to IncrementalChecker class update.

Addresses #67

@ajainarayanan
Copy link
Contributor Author

@johnnyreilly Changes for adding auto fix as a flag. Let me know if I had missed anything.

@johnnyreilly
Copy link
Member

Thanks for this! @piotr-oles what's your feeling on merging this? It's not a feature that I would use but clearly there's some demand out there for this.

I know you expressed reservations so I don't want to merge unless I know you're okay with this!

@piotr-oles
Copy link
Collaborator

Thank you @ajainarayanan for your work! I was afraid that it will add complexity to the plugin but looks like it doesn't. Please add an integration test for tslintAutoFix: true case and update README.md :)

@ajainarayanan
Copy link
Contributor Author

ajainarayanan commented Oct 25, 2018

@piotr-oles @johnnyreilly This is was a tricky change. Have taken an approach that shouldn't affect the other tests.

There is still one problem with should fix linting errors with tslintAutofix flag set to true test. It lints other files as well even though I am explicitly passing in the entry point. Don't know what I am missing but right now my brain is fried and can't think of a reason. Will update the pull request when I have sometime. If you happen to identify the reason as to why it updates other files please do point out and I will fix it.

UPDATE:
This is fixed now.

package.json Outdated Show resolved Hide resolved
test/integration/index.spec.js Show resolved Hide resolved
@johnnyreilly
Copy link
Member

@ajainarayanan could you merge in the upstream changes please?

@ajainarayanan
Copy link
Contributor Author

@johnnyreilly Have updated.

test/integration/index.spec.js Outdated Show resolved Hide resolved
@ajainarayanan
Copy link
Contributor Author

@piotr-oles @johnnyreilly Have updated the pull request. Can you give it another review.

@johnnyreilly
Copy link
Member

Just merged in upstream changes @ajainarayanan.

@piotr-oles are you happy with the amends?

@ajainarayanan
Copy link
Contributor Author

@johnnyreilly @piotr-oles I know its a little ridiculous to request some changes in an open source library and expect a release to happen right away but I am blocked by this change (or a new version of fork-ts-checker) to integrate prettier into my project.

Do we know tentatively when the pull request will be merged and a release will be cut? If any further changes are required please do let me know and I will make it.

@johnnyreilly
Copy link
Member

Hey @ajainarayanan , sorry this is a bit of a frustration for you. From my perspective I'm fine for this to be merged and shipped now. Given @piotr-oles requested changes (which you've generously done!) I'd like him to confirm he's happy with the work rather than me merge directly.

If there's no response from him by the weekend then ping me again, maybe we can proceed without him. But that's not my preference.

Apologies for the delay; I understand the frustration.

@johnnyreilly
Copy link
Member

Okay let's do this! Do you want to update the package.json and the CHANGELOG.md? Then I can cut a release.

@ajainarayanan
Copy link
Contributor Author

@johnnyreilly Thanks! Have updated both. Could you give it a review?

@johnnyreilly
Copy link
Member

Looks good - could you put the Default value of the field in the README.md please? Then we're good to go!

@piotr-oles
Copy link
Collaborator

Thank you for your work and sorry for blocking this PR. Like @johnnyreilly said, please add default value to the README.md and then we're ready for release :)

@ajainarayanan
Copy link
Contributor Author

@piotr-oles @johnnyreilly Updated. Thanks for the review.

@johnnyreilly johnnyreilly merged commit 97c86d6 into TypeStrong:master Nov 7, 2018
@johnnyreilly
Copy link
Member

Thanks! Should be available shortly

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

Successfully merging this pull request may close these issues.

3 participants