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

Handle repeated options. #2021

Merged
merged 2 commits into from
May 21, 2018
Merged

Handle repeated options. #2021

merged 2 commits into from
May 21, 2018

Conversation

abellgithub
Copy link
Contributor

Close #1941

@abellgithub abellgithub requested a review from chambbj May 16, 2018 17:21
doc/pipeline.rst Outdated
Each of these output sets is called a point view. Point views are carried
through a PDAL pipeline individually. Some writers can produce separate
output for each input point view. These writers use a placeholder character
(#) in the output filename which is replaced by a incrementing integer for
Copy link
Member

Choose a reason for hiding this comment

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

Should read replaced by an incrementing integer.

Copy link
Member

@chambbj chambbj left a comment

Choose a reason for hiding this comment

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

@abellgithub I think this mostly looks good. So, in limited testing, it looks like both comma-separated options and JSON arrays are allowed for multiple options. Probably the comma-separated stuff is stage specific (I've only tested with range filter). Is this the intended behavior? I don't mind, but I did notice that in the colorization example you explicitly change it from comma-separated repeated options to the array.

@abellgithub
Copy link
Contributor Author

abellgithub commented May 21, 2018

If an option is bound to vector<string>, it can be parsed by ProgramArgs as a comma-separated list.

Theoretically, you can handle any argument type that takes a vector<type> as you like by adding appopriate code. But with this change you can let the JSON parser break the options up for you and then add them one at a time. There are a bunch of places where we take vector<string> as options, and these can be handled either way. I changed the colorization example just to provide an example using the syntax that's now supported.

@chambbj
Copy link
Member

chambbj commented May 21, 2018

LGTM

@abellgithub abellgithub merged commit d1a5e7d into master May 21, 2018
@abellgithub abellgithub deleted the issue-1941 branch June 27, 2018 16:10
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.

None yet

2 participants