-
Notifications
You must be signed in to change notification settings - Fork 107
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
Raise error on faulty result file and missing report steps #5724
Conversation
532b6c4
to
9231ac3
Compare
Codecov Report
@@ Coverage Diff @@
## main #5724 +/- ##
==========================================
- Coverage 79.98% 79.98% -0.01%
==========================================
Files 362 362
Lines 22520 22523 +3
Branches 989 989
==========================================
+ Hits 18012 18014 +2
- Misses 4240 4241 +1
Partials 268 268
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
8632c4a
to
4fbde18
Compare
src/ert/parsing/lark_parser.py
Outdated
continue | ||
|
||
args = args[0:1] | ||
# continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why continue commented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, should be removed instead. Omiting the continue
will make it just ignore superfluous tokens, but still give an error about them.
f"The RESULT_FILE:{res_file} setting for {name} is invalid - " | ||
"must have an embedded %d - and be a relative path" | ||
result_file_context = next( | ||
x for x in gen_data if x.startswith("RESULT_FILE:") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would error_message
be a more meaningful name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not fully sure about this. This variable is a ContextValue
, which has context information about the location of the word in the config file. I did type annotate the variable to make it a bit more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looking good! just a few minor comments, and one question
ErrorInfo( | ||
filename=result_file_context.token.filename, | ||
message=f"The RESULT_FILE:{res_file} setting for {name} is " | ||
f"invalid - must have an embedded %d - and be a relative path", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just a litte suggestion regarding punctuation, you can skip this change if you want:
f"invalid - must have an embedded %d - and be a relative path", | |
f"invalid - must have an embedded %d and be a relative path", |
raise ConfigValidationError.from_info( | ||
ErrorInfo( | ||
filename=gen_data.keyword_token.filename, | ||
message="The GEN_DATA keywords must have a REPORT_STEPS:xxxx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again just language and perhaps make the argument we expect more explicit using quotes:
message="The GEN_DATA keywords must have a REPORT_STEPS:xxxx" | |
message="The GEN_DATA keywords must have 'REPORT_STEPS:xxxx'" |
@@ -362,7 +362,8 @@ def _handle_includes( | |||
filename=config_file, | |||
).set_context(error_context) | |||
) | |||
continue | |||
|
|||
args = args[0:1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit unexpected / out of place. what is happening here, and how is it related to the issue this PR fixes? or does this fall under camp-site rule?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second look, this change makes sense but is not directly related to this PR, just a very tiny thing I noticed doing this, so I guess it does fall under the camp-site rule.
If an include has multiple arguments, it will now keep only the first, and (1) lint that the rest are superfluous, and (2) potentially also lint errors pertaining to the actually included file. With the continue in place, it will only do (1) because the continue stops the execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put it into its own commit, add the context you just wrote in the commit message body, write a test if you can, and then you will have earned a(nother) gold star from me : ) (and can merge it in this PR, i think)
# we manually raise an "empty" error to make | ||
# this raise an assertion error that can be | ||
# acted upon from assert_that_config_does_not_lead_to_error | ||
raise ConfigValidationError(errors=[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems hacky / funky upon first read 😬 in a function called asser_that_config_leads_to_error
, you would expect that the config file contents you pass in always lead to an error, otherwise you would use a different function. so why the need to artificially raise an error?
seems like it might cause false positives when using that function with a config file that one thinks should lead to an error, but it don't 🤔
EDIT: i now see the catch in line 132. i guess it protects exactly against what i'm describing here 😅
If an include has multiple arguments, it will now keep only the first, and (1) lint that the rest are superfluous, and (2) potentially also lint errors pertaining to the actually included file. With the continue in place, it will only do (1) because the continue stops the execution.
7ea4d4b
to
e08d7a6
Compare
Issue
Resolves #5712
Approach
Raise localized error upon faulty
RESULT_FILE
without%d
, or missingREPORT_STEPS
ofGEN_DATA
entry.Pre review checklist
Adding labels helps the maintainers when writing release notes. This is the list of release note labels.