Replace os.path.join with pathlib#13583
Open
berland wants to merge 5 commits into
Open
Conversation
c18f425 to
2a060c4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13583 +/- ##
=======================================
Coverage 89.52% 89.52%
=======================================
Files 464 464
Lines 32775 32779 +4
=======================================
+ Hits 29343 29347 +4
Misses 3432 3432
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR replaces os.path.join(...) usages with pathlib.Path-based joining across ERT/Everest runtime code and a broad set of tests, to enable adding Ruff rule PTH118 (discouraging os.path.join) without large follow-up noise.
Changes:
- Refactor many path constructions from
os.path.jointoPath(...) / ...(and relatedPath(...)conversions) in both source and tests. - Adjust tests to expect string vs
Pathvalues where APIs still operate onstrpaths. - Enable Ruff rule
PTH118inpyproject.toml.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/everest/utils/utils.py | Switches relpath helper to return a Path. |
| tests/everest/test_templating.py | Uses Path joining for loading template_render.py. |
| tests/everest/test_res_initialization.py | Updates expected install_data arglist values (drops ./ prefix). |
| tests/everest/test_repo_configs.py | Reworks repo config discovery using Path.rglob. |
| tests/everest/test_logging.py | Converts log/output path handling to Path. |
| tests/everest/test_everest_output.py | Uses Path for saved config path and move destination. |
| tests/everest/test_config_file_loader.py | Aligns path expectations using Path joins and string conversions. |
| tests/ert/unit_tests/run_models/conftest.py | Uses Path.mkdir(parents=True) for dummy runpath directory structure. |
| tests/ert/unit_tests/resources/test_templating.py | Uses Path joining for loading template_render.py. |
| tests/ert/unit_tests/resources/test_shell.py | Introduces SHELL_SCRIPTS Path and updates script imports to use it. |
| tests/ert/unit_tests/gui/tools/plot/conftest.py | Uses Path joining for test-data directory path. |
| tests/ert/unit_tests/forward_model_runner/test_forward_model_step.py | Converts executable paths to stringified Path joins. |
| tests/ert/unit_tests/config/test_forward_model.py | Updates expected executable path formatting with Path. |
| tests/ert/unit_tests/config/test_ert_config.py | Converts workflow/script temp paths to Path usage. |
| tests/ert/unit_tests/config/test_create_forward_model_json.py | Builds executable paths with Path and str(...). |
| tests/ert/unit_tests/config/config_dict_generator.py | Converts generated config paths to str(tmp / ...). |
| tests/ert/ui_tests/gui/conftest.py | Uses Path joining for copying poly example test-data. |
| tests/ert/ui_tests/cli/test_update_with_rft_observations.py | Uses Path joining for copying rft example test-data. |
| tests/ert/ui_tests/cli/test_cli.py | Uses Path joining for observation config absolute path. |
| tests/ert/conftest.py | Adds Path typing for source_root and updates test-data copy paths. |
| src/everest/config/validation_utils.py | Uses Path joining in path validation and absolute path creation. |
| src/everest/config/server_config.py | Computes session dir using Path.resolve() and Path joining. |
| src/everest/config/install_data_config.py | Uses Path joining for non-absolute install-data sources. |
| src/everest/config/everest_config.py | Uses Path joining for output/simulation dir computations. |
| src/ert/storage/migration/to8.py | Uses Path joining when listing observation keys directory. |
| src/ert/run_models/everest_run_model.py | Uses Path joining for .res_runpath_list path. |
| src/ert/resources/shell_scripts/symlink.py | Uses Path for target existence checks prior to symlink creation. |
| src/ert/resources/shell_scripts/move_file.py | Accepts Path inputs and uses Path operations internally. |
| src/ert/resources/shell_scripts/delete_directory.py | Uses Path for existence checks and joined file paths in traversal. |
| src/ert/resources/shell_scripts/copy_file.py | Uses Path joining to build target file paths. |
| src/ert/resources/shell_scripts/copy_directory.py | Uses Path joining to build target directory path when target is a dir. |
| src/ert/resources/shell_scripts/careful_copy_file.py | Uses Path joining to build target file paths. |
| src/ert/plugins/hook_implementations/jobs.py | Converts rendered job directories to Path and iterates with iterdir(). |
| src/ert/config/parsing/lark_parser.py | Uses Path for include-directory handling and absolute include resolution. |
| src/ert/config/parsing/config_schema_item.py | Resolves PATH/EXECUTABLE schema tokens using Path operations. |
| src/ert/config/observation_config_migrations.py | Uses Path for resolving OBS/INDEX file paths before storing as string. |
| src/ert/config/ert_config.py | Uses Path for default paths and directory file enumeration; converts to str where needed. |
| src/ert/config/_observations.py | Accepts Path for CSV filename and resolves relative paths via Path. |
| src/_ert/forward_model_runner/reporting/file.py | Uses Path joining when reporting stderr file location. |
| src/_ert/forward_model_runner/io/init.py | Reworks executable checks to use Path for candidate construction and checks. |
| pyproject.toml | Enables Ruff rule PTH118. |
Comment on lines
+20
to
+22
| directory / file | ||
| for file in directory.iterdir() | ||
| if (directory / file).is_file() |
Comment on lines
1424
to
1427
| elif not path.isabs(content_dict[ConfigKeys.RUNPATH_FILE]): | ||
| content_dict[ConfigKeys.RUNPATH_FILE] = path.normpath( | ||
| path.join(config_dir, content_dict[ConfigKeys.RUNPATH_FILE]) | ||
| content_dict[ConfigKeys.RUNPATH_FILE] = str( | ||
| Path(config_dir) / content_dict[ConfigKeys.RUNPATH_FILE] | ||
| ) |
Comment on lines
36
to
47
| careful_copy_file = import_from_location( | ||
| "careful_copy", | ||
| os.path.join(SOURCE_DIR, "src/ert/resources/shell_scripts/careful_copy_file.py"), | ||
| SHELL_SCRIPTS / "careful_copy_file.py", | ||
| ).careful_copy_file | ||
| mkdir = import_from_location( | ||
| "make_directory", | ||
| os.path.join(SOURCE_DIR, "src/ert/resources/shell_scripts/make_directory.py"), | ||
| SHELL_SCRIPTS / "make_directory.py", | ||
| ).mkdir | ||
| careful_copy_file = import_from_location( | ||
| "careful_copy", | ||
| os.path.join(SOURCE_DIR, "src/ert/resources/shell_scripts/careful_copy_file.py"), | ||
| SHELL_SCRIPTS / "careful_copy_file.py", | ||
| ).careful_copy_file |
Contributor
Author
There was a problem hiding this comment.
removed the duplicate
One change in test directories as we do not need to conserve "./" as a path prefix, this was present in tests just as an artifact of how os.path.join works when joining with ".".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Resolves https://docs.astral.sh/ruff/rules/os-path-join/
Approach
Try limiting the changes to only replacing
os.path.joinand accepting that there are lots of obvious pathlib improvements that still can be done to the changed lines - leave these for later for easier review. The main motive is to be able to add PTH118 as a ruff rule as soon as possible, and then do more.git rebase -i main --exec 'just rapid-tests')When applicable