Remove blacklist/whitelist Terms #622
Conversation
Signed-off-by: Hunter L. Allen <[email protected]>
Signed-off-by: Hunter L. Allen <[email protected]>
|
This doesn't sound like a good change: the packages on the blacklist are not denied anything. They are ignored by the tool during topological ordering, which is something else. |
Which term would you suggest instead? Maybe "skip list"? |
|
Perhaps |
|
In |
| clr("@{cf}Whitelisted Packages:@| @{yf}{allowlisted_packages}@|"), | ||
| clr("@{cf}Blacklisted Packages:@| @{yf}{denylisted_packages}@|"), |
There was a problem hiding this comment.
The labels would need to be updated as well.
| # Select the blacklist | ||
| blacklist = DEVEL_LINK_PREBUILD_BLACKLIST if prebuild else DEVEL_LINK_BLACKLIST | ||
| # Select the denylist | ||
| denylist = DEVEL_LINK_PREBUILD_BLACKLIST if prebuild else DEVEL_LINK_BLACKLIST |
| @@ -138,43 +138,43 @@ This can be done with the ``--extend`` option like so: | |||
| Whitelisting and Blacklisting Packages | |||
|
These options are most closely related to the ignoring behavior in colcon. It dictates which packages are built or not by default when you don't specify anything else. It's part of the persistent |
| If the *blacklist* is non-empty, it will filter the packages to be built in all cases except where a given package is named explicitly. | ||
| This means that blacklisted packages will not be built even if another package in the workspace depends on them. | ||
| If the *denylist* is non-empty, it will filter the packages to be built in all cases except where a given package is named explicitly. | ||
| This means that denylisted packages will not be built even if another package in the workspace depends on them. | ||
|
|
||
| .. note:: | ||
|
|
||
| Blacklisting a package does not remove it's build directory or build |
|
@wjwwood thanks for the review, I'll catch those in my next pass. @dirk-thomas / @wjwwood Glad to hear other suggestions for that one, as I'm not quite sure (I don't really use the |
|
|
Also make sure to properly update the verbs. "blacklisted"/"whitelisted" are verbs, but "allowlisted/denylisted"? If you're going with "ignorelist" and "selectlist", use the proper verb forms like "ignored" and "selected". @wjwwood / @dirk-thomas : Is this change going to be rolled out in released distros? Will there be some kind of backwards compatibility / deprecation warnings / user config migration? |
@mintar Yeah, that's a good point. For context, I just looked up what Linux kernel is using to replace "blacklist/whitelist" and applied it in this patch, but it certainly isn't proper English and doesn't really sound all that great (or as @gavanderhoorn pointed out earlier make all that much sense). and seem to be the most in-line with the way |
I don't know how we'd land the change just yet. I think it would be best to perhaps allow the old arguments to continue to work as aliases to the new arguments, but not document them and perhaps have a deprecation warning. If we released it, it would likely only impact the Python3 version of catkin_tools I still need to release and so it would limited to some of the newer Ubuntu distributions and only if you use the python3 version in older distros (as it is available). It has no correlation with ROS distributions, since it is a pure Python tool. |
Also would affect pip installs, but those a probably a smaller minority. I agree some config migration / backwards compatibility / deprecation would be good. |
|
I looked at this again for a while this afternoon. I want to get it in a better state in terms of backwards compatibility before I push some new changes. I just wanted to let you know @allenh1 that I was making fixups and stuff so you didn't do the same. |
|
@wjwwood thanks for letting me know, sorry for leaving this one get stale. I will have some time this weekend if you need me to do any modifications. |
|
It's ok, I'll let you know. |
|
Closed in favor of #712. |
Given the recent wave of renaming terms to be more inclusive, I thought it might be worth removing the terms "blacklist" and "whitelist".
This PR replaces the terms with "denylist" and "allowlist" respectively (one of the proposed replacements for these terms for Linux kernel), but I would be happy to change this patch to use other terms upon request.