adding better terminology for selecting which packages to build

catkin / catkin_tools

Command line tools for working with catkin

Apache License 2.0

163 stars 146 forks source link

adding better terminology for selecting which packages to build #712

Closed jbohren closed 2 years ago

jbohren commented 2 years ago

Overview:

Changes all instances of "whitelist" to "buildlist"
Changes all instances of "blacklist" to "skiplist"
Permits usage of old cli arguments
Warns the user when old cli arguments are used

@allenh1 I appreciate you putting the time in to address the poorly chosen terminology present in catkin_tools. This PR adds terminology which I think is clearer and better-suited for this application than "allow" and "deny". Since you made the initial PR, I added you as a coauthor to the main commit in this PR. Let me know if you want me to remove you from that commit.

Note the asciinema videos in the documentation still display the old terminology.

This should supersede:

allenh1 commented 2 years ago

Great -- thanks for following up on this.

wjwwood commented 2 years ago

I support this change, but I don't have time to take this pull request on, sorry. Maybe @timonegk can give it a review and merge.

jbohren commented 2 years ago

@timonegk

Currently, the configuration yaml file also contains the blacklist/whitelist terms. In metadata.py, there is a function to migrate metadata. The migration to the new terms should be added there.

Yeah that's a good point. See migration in 46126ce

Also, the CI is currently failing. Could you look into that?

Sorry, I don't have the bandwidth for that.

timonegk commented 2 years ago

Okay, then I will continue to work on the things that I commented. Do you mind giving me push access to your branch so that I could keep it in this pull request?

timonegk commented 2 years ago

Thanks!

timonegk commented 2 years ago

On the same note, I also changed the name of the base branch from master to main.

timonegk commented 2 years ago

@wjwwood I just tagged a new release (0.8.3)

jbohren commented 2 years ago

@wjwwood @timonegk thanks for following through on this!

wjwwood commented 2 years ago

I didn't see it on PyPi, so I pushed it there too.

Debs should be available in a few minutes.

timonegk commented 2 years ago

Oh yes, I forgot about PyPi. Thanks for both!

MatthijsBurgh commented 2 years ago

This has now been released as 0.8.3, but this a breaking change. So this should have been a major release. 😠

timonegk commented 2 years ago

Nothing here is a breaking change.

MatthijsBurgh commented 2 years ago

It is for plugins. For example catkin_tools_document. As it accesses the black/whitelist attributes of Context, see https://github.com/mikepurvis/catkin_tools_document/pull/25/

timonegk commented 2 years ago

Ah yes, that's true... Sorry 😕

icefoxen commented 2 years ago

It is also a breaking change because it auto-migrates config files to the new terms... so after you use the new version once, old versions of catkin will no longer build your project.

timonegk commented 2 years ago

Okay, then I would do the following to mitigate the issues:

Add a new patch release that adds back the black/whitlist attributes of Context, migrates config files back to the old terms and can deal with both types of arguments. That should add back all breaking changes.
Release a new version 1.0.0 with the migrated config and Context class.

What do you think of this plan? Did I forget anything?