hmil / tslint-override

TSLint plugin bringing support for the override keyword
MIT License
27 stars 6 forks source link

Add support for capital @Override decorators #18

Closed ChiriVulpes closed 5 years ago

ChiriVulpes commented 5 years ago

How about we try #17 again

Closes #14

Sorry about the git mess 😕

Original PR text:

Example usage:

{
    "extends": [
        "tslint-override"
    ],
    "rules": {
        "explicit-override": [ true, "decorator", "@Override" ]
    }
}

Note: This changes the decorator matcher to only work with the decorator of the correct casing. With that in mind, it may be a good idea to add another warning if we see a decorator with the wrong casing, and maybe change those to the correct casing? Should that be an additional option, tho?

Another note: I was going to write some tests for this but I wasn't quite sure how I should do it (especially since I wasn't sure how we want to handle the incorrect casing thing).

ChiriVulpes commented 5 years ago

So for the tests:

  1. The test overridden method that has @Override on it to begin with doesn't pass the decorator test now (since the rule now requires specifying you're using the capital version, or else assumes @override by default).

    • Should we add a new rule & fixer for incorrect casing like this?
  2. There's no tests for the @Override option

    • I assume we want to do that by cloning the tslint.decorator.json file again, maybe tslint.decorator.uppercase.json? And then make a fixed file for that as well? FYI, I don't think I'll be able to edit the test.sh file to include those tests, if I add them, based on what happened by the upstream merge last PR 😜
hmil commented 5 years ago

Glad to see it worked.

So let's just clearly state what you want to do in this PR.

The fixer

I think originally, you wanted this plugin to support a default casing for the fixer. I agree with this feature. The fixer shouldn't generate bad code. It should help stay consistent across the codebase. This applies to both annotations and JSDoc.

The linter

I don't think a change in behavior for the linter is required. Here is why:

JSDoc

JSDoc tags are case insensitive. Therefore, enforcing a case is not the job of this lint rule, it would be the job of another lint rule which would watch for casing inconstancies in JSDoc tags.

Annotation

The annotation is an actual typescript identifier. Therefore, in order for the code to be valid, a symbol for that identifier must be defined in the current scope. tslint-override provides a helper to make this symbol available: import "tslint-override/register" near your application's entry point will make the override symbol available across the codebase. Alternatively, one can define the override or Override function somewhere in her code. If the override keyword has the wrong casing, then the typescript compiler will complain because there is no function in the scope with that name.

Therefore, we can say that all of the cases of invalid case (pun intended) are handled by external tools.


I will add some inline suggestions to modify the PR accordingly.

ChiriVulpes commented 5 years ago

I didn't realise capital jsdoc tags were even a thing, that's why I stripped support for that out of the initial version. I'll add it back

ChiriVulpes commented 5 years ago

Note: checkJSDocChild does not currently support @Override

hmil commented 5 years ago

Note: checkJSDocChild does not currently support @Override

Weird, I didn't notice but that's a bug.

Thanks for your contribution!