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

Refs #17135 - Replace CSV parser in List Normalizer #246

Merged
merged 2 commits into from Jul 18, 2017

Conversation

Projects
None yet
4 participants
@mbacovsky
Member

mbacovsky commented Jun 22, 2017

The ruby CSV parser causes some usability problems when used to parse list options for more details see [1].
The new parser should be more flexible in syntax and closer to shell quoting conventions.

[1] theforeman/hammer-cli-foreman#312

@mbacovsky

This comment has been minimized.

Show comment
Hide comment
@mbacovsky

mbacovsky Jun 22, 2017

Member

Examples of the required CSV syntax can be seen in the tests. I'd be glad for suggestions on adding other inputs to the tests.

Member

mbacovsky commented Jun 22, 2017

Examples of the required CSV syntax can be seen in the tests. I'd be glad for suggestions on adding other inputs to the tests.

@orrabin

This comment has been minimized.

Show comment
Hide comment
@orrabin

orrabin Jun 22, 2017

Member

I tried this with theforeman/hammer-cli-foreman#312 all these options worked correctly for me:
--override-value-order 'domain,os'
--override-value-order "'domain,os', hostgroup"
--override-value-order "'domain,os', 'hostgroup'"
--override-value-order "'domain,os', 'hostgroup, os', fqdn"

Member

orrabin commented Jun 22, 2017

I tried this with theforeman/hammer-cli-foreman#312 all these options worked correctly for me:
--override-value-order 'domain,os'
--override-value-order "'domain,os', hostgroup"
--override-value-order "'domain,os', 'hostgroup'"
--override-value-order "'domain,os', 'hostgroup, os', fqdn"

@tstrachota

Works as expected, @mbacovsky
I have one question about the error messages, see comments in the code, please.

Show outdated Hide outdated lib/hammer_cli/csv_parser.rb
Show outdated Hide outdated test/unit/csv_parser_test.rb
@mbacovsky

This comment has been minimized.

Show comment
Hide comment
@mbacovsky
Member

mbacovsky commented Jul 12, 2017

@tstrachota, updated

@tstrachota

This comment has been minimized.

Show comment
Hide comment
@tstrachota

tstrachota Jul 18, 2017

Member

👍 thanks @mbacovsky

Member

tstrachota commented Jul 18, 2017

👍 thanks @mbacovsky

@tstrachota tstrachota merged commit b1cf796 into theforeman:master Jul 18, 2017

1 check passed

default Job result: SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment