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

feat: add "no-fix" option #16

Closed
wants to merge 1 commit into from
Closed

Conversation

aleclarson
Copy link
Contributor

This is super useful when using vscode-tslint.

{
  "rules": {
    "no-focused-tests": [true, "no-fix"],
    "no-disabled-tests": [true, "no-fix"],
  }
}

@kaiza
Copy link
Owner

kaiza commented Jan 16, 2019

This has been suggested in the past but it's possible to disable rules from auto-fixing in vscode:

#14

@kaiza kaiza closed this Jan 16, 2019
@aleclarson
Copy link
Contributor Author

That's not practical. I do not wish to update two sources of truth whenever I use a new rule. Not to mention I'm already using dozens of rules. The opt-in feature does not fit my needs, just like this comment. Would you reconsider, please? 😄

@kaiza
Copy link
Owner

kaiza commented Jan 16, 2019

Because it's an issue caused by the editor I believe it should be fixed there as per the discussion in that thread. I don't really think it makes sense that every rule that's potentially problematic should have to implement this. As far as I'm aware none of the other commonly used ones have it.

@aleclarson
Copy link
Contributor Author

aleclarson commented Jan 16, 2019

Yeah, makes sense. In the meantime, I'll just use my own fork. 😉

@dhhyi
Copy link

dhhyi commented Jan 19, 2019

It's not just the IDE environment, that can't properly work with self-unignoring tests. We also use https://github.com/okonet/lint-staged for autofixing all small issues. It really doesn't help if failing ignored tests get automatically re-included all the time.

@aleclarson If you don't need newer features, you can just stick with version 1.4.0 of this library. Of course then you have a problem with tools auto-upgrading your dependencies to the newest versions. 😅

@aleclarson
Copy link
Contributor Author

If you don't need newer features, you can just stick with version 1.4.0 of this library.

@dhhyi Not sure what you mean? This is my PR, so of course I want this feature.

@dhhyi
Copy link

dhhyi commented Jan 19, 2019

If you don't need newer features, you can just stick with version 1.4.0 of this library.

@dhhyi Not sure what you mean? This is my PR, so of course I want this feature.

Yes, but 1.4.0 didn't have this commit: 2909465
It made all rules fixable by default. Before that it was configurable like you suggested.

@aleclarson
Copy link
Contributor Author

@dhhyi Oh, okay.

Maybe when the severity equals "warning", auto-fixing should be disabled. Thoughts @kaiza?

@kaiza
Copy link
Owner

kaiza commented Jan 21, 2019

@dhhyi I kinda think that if a tool provides auto-fixing abilities it should give the ability to opt out of fixing certain rules. Also, if you want to be able to disable tests with xdescribe/xit, then why would you have a lint rule for it? Or are you saying you add that rule with severity warning?

I could get on board with the proposal to disable auto-fixing when severity is 'warning'. Happy for you to update the PR @aleclarson.

@aleclarson
Copy link
Contributor Author

Done 👍

@aleclarson
Copy link
Contributor Author

Hmm.. the force-push isn't showing up.

@dhhyi
Copy link

dhhyi commented Jan 21, 2019

@dhhyi I kinda think that if a tool provides auto-fixing abilities it should give the ability to opt out of fixing certain rules. Also, if you want to be able to disable tests with xdescribe/xit, then why would you have a lint rule for it? Or are you saying you add that rule with severity warning?

@kaiza Yes. We only use it with warning level only. In a bigger code base it should report when somebody knowingly ignored some test cases to fix them later. The behavior in version 1.4.0 was exactly fitting our needs. If auto-fixing is disabled for warning level, this would also be fine.

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.

None yet

3 participants