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

Remove .DATA suffix from REFCASE path #5245

Merged
merged 1 commit into from
Apr 19, 2023

Conversation

andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Apr 18, 2023

Retain backwards compatibility where REFCASE had .DATA suffix The suffix is removed regardless, but this allows us to validate presence of refcase .UNSMRY and .SMSPEC files.

Issue
Resolves #5244

Approach
Short description of the approach

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.

@andreas-el andreas-el added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Apr 18, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2023

Codecov Report

Merging #5245 (fe71c01) into main (7f9b58a) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5245      +/-   ##
==========================================
+ Coverage   73.17%   73.18%   +0.01%     
==========================================
  Files         392      391       -1     
  Lines       26963    26871      -92     
  Branches     1965     1965              
==========================================
- Hits        19729    19665      -64     
+ Misses       6670     6642      -28     
  Partials      564      564              
Impacted Files Coverage Δ
src/ert/_c_wrappers/enkf/ensemble_config.py 95.97% <100.00%> (+0.02%) ⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@DanSava DanSava left a comment

Choose a reason for hiding this comment

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

Except, the small comment, looks good 👍

@@ -179,6 +179,9 @@ def _load_refcase(refcase_file: Optional[str]) -> Optional[EclSum]:
if refcase_file is None:
return None

if refcase_file.endswith(".DATA"):
refcase_file = refcase_file[:-5]
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this comment applies here as well: #5247 (comment)

This will remove any suffix present in the filepath/name
Retain backwards compatibility where REFCASE had `.DATA` suffix
The suffix is removed regardless, but this allows us to validate
presence of refcase .UNSMRY and .SMSPEC files later.
Copy link
Collaborator

@oyvindeide oyvindeide left a comment

Choose a reason for hiding this comment

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

LGTM! One minor comment, but no need to action on it

@@ -179,6 +179,9 @@ def _load_refcase(refcase_file: Optional[str]) -> Optional[EclSum]:
if refcase_file is None:
return None

refcase_filepath = Path(refcase_file)
refcase_file = str(refcase_filepath.parent / refcase_filepath.stem)

if not os.path.exists(refcase_file + ".UNSMRY"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outside the scope here, so no need to fix it, but since you are now using pathlib you can do:

if not (refcase_file / ".UNSMRY").exists:

@andreas-el andreas-el merged commit 43ac566 into equinor:main Apr 19, 2023
35 checks passed
@andreas-el andreas-el deleted the refcase_allow_data_in_path branch October 11, 2023 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refcase does not allow .DATA suffix in configuration
4 participants