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

UW-515 File Copy/Link Tool #429

Merged
merged 31 commits into from
Mar 19, 2024
Merged

Conversation

maddenp-noaa
Copy link
Contributor

Synopsis

Add a file copy/link tool to uwtools.

Type

  • Enhancement (adds a new functionality)

Impact

  • This is a non-breaking change (existing functionality continues to work as expected)

Checklist

  • I have added myself and any co-authors to the PR's Assignees list.
  • I have reviewed the documentation and have made any updates necessitated by this change.

@maddenp-noaa maddenp-noaa self-assigned this Mar 14, 2024
docs/sections/user_guide/cli/drivers/fv3.rst Show resolved Hide resolved
docs/sections/user_guide/cli/tools/file.rst Show resolved Hide resolved
format Show resolved Hide resolved
src/uwtools/config/validator.py Show resolved Hide resolved
src/uwtools/drivers/driver.py Show resolved Hide resolved
src/uwtools/tests/test_cli.py Show resolved Hide resolved
Copy link
Contributor

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

This looks great. I have a few questions/suggestions below.

I was also a little surprised not to have seen any edits to the fv3 driver code. Do you think it may be worthwhile at some point to call the new FileCopier and FileLinker tasks from the drivers? Perhaps we want to wait until there are 3 instances again, like we did with the driver development refactoring? Maybe there's something I'm missing about why we wouldn't want to use these standalone tasks from the drivers?

docs/sections/user_guide/api/index.rst Show resolved Hide resolved
docs/sections/user_guide/cli/tools/file.rst Show resolved Hide resolved
src/uwtools/api/file.py Show resolved Hide resolved
src/uwtools/cli.py Outdated Show resolved Hide resolved
@maddenp-noaa
Copy link
Contributor Author

@christinaholtNOAA fv3 didn't change here because there is already a lot of DRY reuse between fv3 and file. Both use the same schema for validating their file blocks, and both delegate to the filecopy and symlink tasks from uwtools.utils.tasks. Due to the task reuse, methods FV3.files_copied() and FV3.files_linked() each comprise only two Python statements, so there's essentially nothing to factor out -- especially given that one of those two statements is a "branded" logging function, i.e. one that contains information about the fact that this is the FV3 driver, and that includes cycle information. The task names (and therefore log messages) in the file tool have to be more generic, since we don't know what they're being used for. So, given that we're getting IMO helpful specificity in log messages and task names for FV3, and since there's basically no duplication, I don't think this is a good candidate for combination: I think a refactor would be more lines of code, and would involve passing extra arguments to try configure something generic.

@christinaholtNOAA
Copy link
Contributor

Thanks for your thoughts on the reuse. I agree with where you landed.

Copy link
Contributor

@christinaholtNOAA christinaholtNOAA left a comment

Choose a reason for hiding this comment

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

LGTM!

@maddenp-noaa maddenp-noaa merged commit a6d3be0 into ufs-community:main Mar 19, 2024
2 checks passed
@maddenp-noaa maddenp-noaa deleted the UW-515 branch March 19, 2024 00:40
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.

2 participants