mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.79k stars 278 forks source link

refactor: rename blacklist to blocklist and whitelist to allowlist #946

Closed mfederowicz closed 8 months ago

mfederowicz commented 10 months ago

related to #690

I thougt that rename Whitelist to Allowlist and Blacklist to Blocklist (all variable of course respect lower case and upper case)

I dont have any idea how to implement deprecation for old &rule.ImportsBlacklistRule{}, one idea is that to leave empty ImportsBlacklistRule and throw info about deprecation what you think @chavacava

ps. i checked other repos in github with &rule.ImportsBlacklistRule{} and there are 75 repos matching, so we dont have any simple solution :(

mfederowicz commented 10 months ago

Hmm i have idea to add list with deprecated rules and then remove deprecated rules in parseConfig method. I know that is quick and ugly sollution, but better solution will be implement versioning with deprecations list or something like that

chavacava commented 10 months ago

Hi @mfederowicz thanks for the PR. I propose to do these changes in two steps:

  1. Remove white and black list names from the internal code
  2. Remove white and black list names from the external interfaces (config)

Use this PR for the step 1 (the easy one) and discuss how to go on with step 2

mfederowicz commented 10 months ago

ok @chavacava i cleaned up code from my changes related with deprecatedRules list, now there are only changes related with removing white and black lists (of course ImportsBlacklistRule was reverted to old state from master)

now iam listening sugestions/ideas how to remove black list from the external interfaces (config), white list luckly was used only in lists internaly (not in config or dedicated rules)

chavacava commented 10 months ago

Side note: changing the failure messages could be also considered as a breaking change because users might rely on failure messages thus changing them will force users to update their code. Do not worry, I'm okay with the new messages you pushed, my note is just to highlight that managing "breaking changes" is a big deal :)

denisvmedia commented 10 months ago

my note is just to highlight that managing "breaking changes" is a big deal :)

We'll have to put some release notes on that, I think.

mfederowicz commented 9 months ago

@chavacava what is the status, any new sugestions?

chavacava commented 9 months ago

@chavacava what is the status, any new sugestions?

As I commented before, the PR (still) has renamed rules (the imports-blacklist rule renamed as imports-blocklist) Changing the rule's name is a breaking change and my understanding was we agreed on not introducing breaking changes in this first phase of refactoring.

mfederowicz commented 9 months ago

@chavacava what is the status, any new sugestions?

As I commented before, the PR (still) has renamed rules (the imports-blacklist rule renamed as imports-blocklist) Changing the rule's name is a breaking change and my understanding was we agreed on not introducing breaking changes in this first phase of refactoring.

yes but imports-blocklist is new file not rename old one. I can revert testdata for test/import-blacklist_test.go and testdata/imports-blacklist-original.go but this dont have affect to old code :)

chavacava commented 9 months ago

yes but imports-blocklist is new file not rename old one. I can revert testdata for test/import-blacklist_test.go and testdata/imports-blacklist-original.go but this dont have affect to old code :)

Does that mean we will have two rules for the very same check?

mfederowicz commented 9 months ago

ok @chavacava i mean that I created copy of blacklist rule. Now I reverted tests/testdata for blacklist, and add separated test/testdata for blocklist. In near future we should remove Blacklist rule and add some notes about breaking change. If you (user) use development version of package you must know that changes are part of your live :P