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

Separate rules for 'check-overrides' and 'explicit-overrides' #11

Open
TjlHope opened this issue Nov 15, 2018 · 2 comments
Open

Separate rules for 'check-overrides' and 'explicit-overrides' #11

TjlHope opened this issue Nov 15, 2018 · 2 comments
Labels
discussion Discussion about a feature or topic

Comments

@TjlHope
Copy link

TjlHope commented Nov 15, 2018

Firstly, thank you for the plugin, I've been really missing this functionality in TS!

It would be great if the checking of existing @OverRide markers was split out into a separate rule from checking whether a method should have an @OverRide marker added to it.
Essentially the first is enforcing the thing that should really be done by the compiler. Whilst the second is enforcing consistent code style/good coding practice, the job of the linter.

My use case for having separate rules, rather than just config options, is that:

  1. If something is explicitly marked as @OverRide, which doesn't override a parent, then you want a severity:error, and the build to fail.
  2. If there isn't an @OverRide on a parameter that does override a parent, then you want a lower severity, so that you know about it, without the build failing.
@hmil
Copy link
Owner

hmil commented Nov 15, 2018

I don't have a strong objection against splitting the plugin into two rules. However, I do not quite understand the reason why you want to do so.
Both errors seem critical to me. Either you wanted to override something and end up not overriding anything, or you did not intend to override something and ended up overriding something. Actually, the latter seem more dangerous to me because it is the most likely accidental scenario, especially since in JS/TS a private method can be erased by a method of the child class.

Maybe what you describe is more related to disruption: The first case you have to opt-in manually by specifying the override keyword, the second case is enabled implicitly as soon as you add this tslint rule to your config (and potentially finds errors in your whole codebase at once...)

Feel free to open a PR for that.

@TjlHope
Copy link
Author

TjlHope commented Nov 15, 2018

Yes, you make a good point. I think a large part of it is just what I'm used to from java - in retrospect not necessarily a good reason!
The other reason is so I can gradually transition some projects at work, and by not requiring @override on all sub-classes it should lower the threshold to acceptance until I can get the whole team on board.

I'll definitely have a shot at a PR when I next have some time.

@hmil hmil added the discussion Discussion about a feature or topic label Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion about a feature or topic
Projects
None yet
Development

No branches or pull requests

2 participants