Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

update importBlacklistRule to use regular expressions #3504

Merged
merged 8 commits into from
Dec 18, 2018

Conversation

jbsingh
Copy link
Contributor

@jbsingh jbsingh commented Nov 21, 2017

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

The current importBlacklistRule only allows exact match blacklisting. To allow greater flexibility, would like to update this rule to use regular expression pattern match blacklisting. Existing users of the importBlacklistRule would need to make a small update to retain expected behavior, specifically they'd need to place ^ at the start and $ at the end of their existing blacklist entries as shown in the updated documentation.

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

[enhancement] import-blacklist now supports regular expression patterns for blacklisting

@mikehaas763
Copy link

just noticed the label, maybe there is a way to do this in a backward compatible manner?

@jbsingh
Copy link
Contributor Author

jbsingh commented Dec 6, 2017

Thanks Mike for the feedback. When originally coding this, I was hoping to make it backward compatible to eliminate impacts to any existing configuration. Ultimately, I favored the approach chosen for the following reasons:

  1. exact matching can be accomplished by regular expressions so it felt cleaner to use a single more powerful configuration option
  2. supporting both existing exact match configuration in a non-breaking fashion as well as regular expression configuration would introduce some complexity in the code and configuration
  1. the upgrade path is straightforward (anyone using the rule just needs to add anchors ^$ to retain existing functionality, which would be stated in the release notes that users should consult when upgrading)

If there’s community sentiment toward introducing the regular expression capability without a breaking change, I can definitely update the code in this PR. Anyone have any thoughts about this?

@ajafff ajafff mentioned this pull request Dec 6, 2017
4 tasks
@adidahiya
Copy link
Contributor

I'd prefer to do this in a backwards compatible manner. Regex search is slower than indexOf and I think the extra complexity of supporting both configuration types is worth it here.

@jbsingh
Copy link
Contributor Author

jbsingh commented Jan 10, 2018

@adidahiya @ajafff
I made changes so that the solution is backwards compatible, but have not pushed the updates. I wanted to first find out what the protocol is. Since the changes were previously approved, should I wait for additional feedback or proceed with the backwards compatible solution?

@adidahiya
Copy link
Contributor

Please proceed with the backwards compatible solution.

@krawetko
Copy link

merge?

@giladgray giladgray modified the milestones: TSLint v5.10, TSLint 5.x Jul 27, 2018
@ericanderson
Copy link
Member

I'm sorry this took us so long to get to. It seems to be a bit at odds with import-blacklist as it exists now. Can you update it?

@JoshuaKGoldberg
Copy link
Contributor

Tangentially, this should also fix #2806

@jbsingh
Copy link
Contributor Author

jbsingh commented Dec 17, 2018

I should be able to push an update within the next day or two.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Code LGTM. Nice job merging this in!

Docs are a little light but that's fine (it's been open for quite some time). We should consider this rule to be high priority for providing examples of the new options format (#3100, also #1329).

@leachiM2k
Copy link

Hello guys, thank you for merging this PR. This is exactly what we've needed.
But I have a strange issue with this PR. I can't find the implemented option (and docs) in the current 5.12.1 release on npm.

@adidahiya
Copy link
Contributor

@leachiM2k it wasn't released with 5.12.1. You can find out if a commit is in a release by clicking on the hash in the merge message:

image

and then looking at the tags list here:

image

there are no tags, so it is unreleased. this will be released in the next minor version.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants