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

Allow quoted substrings as part of the FORWARD_MODEL arglist in new parser #5327

Merged
merged 1 commit into from
May 8, 2023

Conversation

DanSava
Copy link
Contributor

@DanSava DanSava commented May 2, 2023

Issue
Resolves #5255

Approach
Allow quoted substrings as part of the FORWARD_MODEL arglist in new parser

Pre review checklist

  • Added appropriate release note label
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Updated documentation
  • Ensured new behaviour is tested

Adding labels helps the maintainers when writing release notes. This is the list of release note labels.

@DanSava DanSava force-pushed the forward_model_arg_list_new_parser branch from 86a058d to 8bdb5ca Compare May 3, 2023 06:48
@DanSava DanSava self-assigned this May 3, 2023
@@ -92,11 +92,12 @@ def test_parsing_forward_model_with_quotes_does_not_introduce_spaces():
comment interpretation, quotation marks are used"""

test_config_file_name = "test.ert"
str_with_quotes = 'smt/<foo>"/bar"/xx/"t--s.s"/yy/"z/z"/oo'
Copy link
Contributor

@oysteoh oysteoh May 5, 2023

Choose a reason for hiding this comment

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

is it relevant to account for "smt/<foo>'/bar'/xx/'t--s.s'/yy/'z/z'/oo" as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good question, I thought about accounting for it, but the old parser does not allow using ' to escape things like 'file--name.txt' so I did not add the functionality.

I can do it for the new parser if you think it is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea - if the old code didn't allow for it then i suggest we keep your PR as-is 👍👍

@DanSava DanSava force-pushed the forward_model_arg_list_new_parser branch from 8bdb5ca to 3f34600 Compare May 5, 2023 17:02
@DanSava DanSava force-pushed the forward_model_arg_list_new_parser branch from 3f34600 to 9db8b07 Compare May 8, 2023 07:49
@DanSava DanSava force-pushed the forward_model_arg_list_new_parser branch from 9db8b07 to 102b1bb Compare May 8, 2023 07:57
@DanSava DanSava merged commit a3a212f into equinor:main May 8, 2023
37 checks passed
@DanSava DanSava deleted the forward_model_arg_list_new_parser branch May 8, 2023 10:13
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.

FORWARD_MODEL arglist does not allow concatenation of arguments
2 participants