Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

Add ban-types rule #181

Merged
merged 1 commit into from
Dec 5, 2018
Merged

Add ban-types rule #181

merged 1 commit into from
Dec 5, 2018

Conversation

armano2
Copy link
Contributor

@armano2 armano2 commented Nov 24, 2018

I ported this rule and tests from tslint, additionally i added some extra tests to check more cases

this rule relies on changes from #174

Copy link
Owner

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question here for a general consensus.
Do we want to 1 for 1 implement the tslint rule?

First:
It seems to me like the configuration is a bit ugly with arrays of arrays.
It's error-prone and unintuitive.
Would prefer something like:

{
  "types": {
	"String": "Use string instead", // custom error message
    "Foo": null // default error message
  }
}

Second:
Do we want to add a fixer to this rule?
I personally think that we should endeavour to add fixers to as many rules as possible.
Esp if the general use case for this rule is to prevent usage of a type, there's probably an alternative.
In this case we could add a config option to specify the alternate type.

@armano2
Copy link
Contributor Author

armano2 commented Nov 26, 2018

@bradzacher yup, i was thinking about this, but i decided to keep interface as is in tslint, i can change it :>
changing options is going to cleanup code https://github.com/bradzacher/eslint-plugin-typescript/pull/181/files#diff-0242bc852e8543fc636ee062b862330bR44 is converting it to this format

fixer will be nice but it will require users to specify to what they want to change it.
how to you want options to look like with fixer?

{
  "types": {
     "String": "string",
     "Foo": null
  }
}

maybe we can change interface a little that, when user is not allowed to specify message, but replace string?

@armano2
Copy link
Contributor Author

armano2 commented Nov 26, 2018

@bradzacher i updated options to follow first of your suggestions

@bradzacher
Copy link
Owner

great work!

Maybe add a third option which is an object config?

{
  "Foo": null, // no custom error
  "Bar": "Bar is deprecated, there is no direct replacement", // custom error only
  "String": { // custom error and an optional fixer!
    "message": "Use string instead",
    "fixWith": "string" // nullable, in case there is no fix...
  },
}

What do you think?

I'm not sure - depends on our intention with this rule.
Is it purely for flagging things that can't be used, or is it about suggesting alternatives for things?

If it's about forcing alternatives, then having the null, and string-only options is counter-productive, because it allows people to not see the fixer option (and it makes your code slightly more complex).

If it's purely for flagging, then the 3 option style is best.

@armano2
Copy link
Contributor Author

armano2 commented Nov 26, 2018

we can add support for all 3 syntaxes, it's not going to be hard, just few ifs.

@armano2
Copy link
Contributor Author

armano2 commented Nov 26, 2018

@bradzacher i added support for all 3 of them :>

Copy link
Owner

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor nits, otherwise LGTM - great work

lib/rules/ban-types.js Outdated Show resolved Hide resolved
docs/rules/ban-types.md Outdated Show resolved Hide resolved
@bradzacher
Copy link
Owner

Almost forgot - don't forget to update the root README.md!
Fix the tests, address the nits and we can merge this

@armano2
Copy link
Contributor Author

armano2 commented Nov 26, 2018

this rule relies on changes from #174

and all tests should pass when #174 is merged

@bradzacher bradzacher merged commit f89e02f into bradzacher:master Dec 5, 2018
@armano2 armano2 deleted the ban-types branch December 5, 2018 01:43
@dannyfritz dannyfritz mentioned this pull request Dec 14, 2018
31 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants