Raise error on faulty result file and missing report steps #5724
Conversation
yngve-sk
force-pushed
the
ensconfig-missing-confignode
branch
3 times, most recently
from
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 |
yngve-sk
force-pushed
the
ensconfig-missing-confignode
branch
3 times, most recently
from
8632c4a
to
4fbde18
Compare
yngve-sk
added
the
release-notes:skip
src/ert/parsing/lark_parser.py
Outdated
| continue | ||
|
|
||
| args = args[0:1] | ||
| # continue |
There was a problem hiding this comment.
why continue commented?
There was a problem hiding this comment.
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.
Would error_message be a more meaningful name?
There was a problem hiding this comment.
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
| 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.
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.
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.
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.
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.
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.
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.
yngve-sk
force-pushed
the
ensconfig-missing-confignode
branch
from
7ea4d4b
to
e08d7a6
Compare
Issue
Resolves #5712
Approach
Raise localized error upon faulty
RESULT_FILEwithout%d, or missingREPORT_STEPSofGEN_DATAentry.Pre review checklist
Adding labels helps the maintainers when writing release notes. This is the list of release note labels.