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

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

Merged
merged 21 commits into from
Jan 15, 2024

Conversation

mfederowicz
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 mfederowicz force-pushed the deprecate-whitelist-blacklist branch from aaa2398 to 97795eb Compare November 30, 2023 19:50
@mfederowicz mfederowicz force-pushed the deprecate-whitelist-blacklist branch from 01996e1 to 1d89e03 Compare December 1, 2023 00:44
@mfederowicz
Copy link
Contributor Author

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)

Copy link
Collaborator

@chavacava chavacava left a comment

Choose a reason for hiding this comment

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

@mfederowicz I've left some comments in the PR.
Please keep in mind that changing the name of a rule is a breaking change (users might need to modify configurations ) As I proposed, this PR should include only internal naming changes.

README.md Outdated Show resolved Hide resolved
RULES_DESCRIPTIONS.md Outdated Show resolved Hide resolved
rule/add-constant.go Outdated Show resolved Hide resolved
rule/imports-blocklist.go Outdated Show resolved Hide resolved
rule/var-naming.go Outdated Show resolved Hide resolved
test/import-blocklist_test.go Outdated Show resolved Hide resolved
@mfederowicz mfederowicz force-pushed the deprecate-whitelist-blacklist branch from 494b300 to 66e1d2a Compare December 1, 2023 19:05
@chavacava
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Contributor Author

@chavacava what is the status, any new sugestions?

@chavacava
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Contributor Author

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

mmcloughlin and others added 5 commits January 14, 2024 23:13
* update tests

* Update testdata/unhandled-error-w-ignorelist.go

* Update testdata/unhandled-error-w-ignorelist.go

---------

Co-authored-by: Denis Voytyuk <[email protected]>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@chavacava chavacava merged commit 9abe06a into mgechev:master Jan 15, 2024
4 checks passed
@mfederowicz mfederowicz deleted the deprecate-whitelist-blacklist branch October 2, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants