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

context: adding append/remove args option #179

Merged
merged 1 commit into from
Apr 3, 2015

Conversation

jbohren
Copy link
Contributor

@jbohren jbohren commented Apr 1, 2015

Fixes #176

@jbohren jbohren force-pushed the feature-context-append-remove branch 3 times, most recently from 190fc96 to 24c9f2e Compare April 2, 2015 02:12

Several configuration options are actually *lists* of objects. For such
options, you can use the ``--append-args`` and ``--remove-args`` options to
append or remove elements from these lists, respectively.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be (is it?) mentioned somewhere that the default behavior is to replace the list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that should be made more clear.

@wjwwood
Copy link
Member

wjwwood commented Apr 2, 2015

Other than the comment +1

@hgaiser
Copy link

hgaiser commented Apr 2, 2015

As mentioned in #152 , I think it would be better to let the default behaviour be to add items to a list and have a --clear-[something] to clear the list. This maintains the same functionality with one less argument (and to be honest, is what I would expect it to do).

@jbohren
Copy link
Contributor Author

jbohren commented Apr 2, 2015

As mentioned in #152 , I think it would be better to let the default behaviour be to add items to a list and have a --clear-[something] to clear the list. This maintains the same functionality with one less argument (and to be honest, is what I would expect it to do).

I agree. The -clear-*list arguments were misleading. They've been renamed to --no-whitelist and --no-blacklist so that they conform to the pattern used by other arguments like --cmake-args / --no-cmake-args.

With respect to making it append by default, making the --whitelist and --blacklist options behave differently from --cmake-args and --make-args can make the interface start getting really confusing. I think adding an extra -a to append isn't that onerous.

The real problem here is that "whitelist" can be a noun or a verb. The argument is meant to be a noun, i.e. you're setting the whitelist but it's equally reasonable at first glance to read it as if you're whitelisting a package. I also tried to clarify this in the documentation: https://github.com/catkin/catkin_tools/blame/master/docs/verbs/catkin_config.rst#L239-252

- calling catkin config -a [ARGS] will append elements to list args
- calling catkin config -r [ARGS] will remove elements from list args
- also fixing context load/save code style
- also fixing line-too-long code style
@jbohren jbohren force-pushed the feature-context-append-remove branch from 24c9f2e to 4e4d7f3 Compare April 2, 2015 23:42
@jbohren
Copy link
Contributor Author

jbohren commented Apr 2, 2015

@wjwwood I've updated the documentation

@wjwwood
Copy link
Member

wjwwood commented Apr 3, 2015

lgtm, thanks.

wjwwood added a commit that referenced this pull request Apr 3, 2015
@wjwwood wjwwood merged commit 6d0fa64 into catkin:master Apr 3, 2015
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.

config: add --append option for list-type configuration options
3 participants