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

Vector input to config file #1069

Merged
merged 18 commits into from
Sep 26, 2024
Merged

Conversation

phlptp
Copy link
Collaborator

@phlptp phlptp commented Sep 6, 2024

From #1067, there is a bit of ambiguity in the handling of config file with vector input and multiple consecutive parameters. For example

option1=[3,4,5]
option1=[4,5,6]

Currently this is handled as if it were

option1=[3,4,5,4,5,6]

But this could be confusing in the case where the input was referring to a vector of vectors.
This PR adds a separator in the sequence to separate the vector so they are two vectors of 3 elements each.
Will need to verify if this change has other side effects. It is a pretty unusual situation.

Copy link

codecov bot commented Sep 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (e4ee3af) to head (dcebf0d).
Report is 37 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##              main     #1069    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           17        17            
  Lines         4546      4627    +81     
  Branches         0       993   +993     
==========================================
+ Hits          4546      4627    +81     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phlptp phlptp requested a review from henryiii September 8, 2024 02:52
@phlptp
Copy link
Collaborator Author

phlptp commented Sep 8, 2024

@henryiii I will merge this Tuesday, unless you have some issues with it.

@pauldmccarthy
Copy link

pauldmccarthy commented Sep 9, 2024

@phlptp This appears to have resolved the issue, thanks very much for looking into it!

edit my comment was a bit premature, as I am still getting inconsistent behaviour when I adjust my example a little bit - will continue the discussion at #1067

@phlptp phlptp merged commit 8c6a73d into CLIUtils:main Sep 26, 2024
55 checks passed
@phlptp phlptp deleted the config_separator_injection branch September 26, 2024 13:02
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.

2 participants