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

Allow users to provide copying of optional files without failing. #41

Merged
merged 7 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .vscode/launch.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
// Use IntelliSense to learn about possible attributes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these vscode files supposed to be committed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They should be. There is nothing here that is user specific.

// Hover to view descriptions of existing attributes.
// For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387
"version": "0.2.0",
"configurations": [
{
"name": "Python Debugger: Current File",
"type": "debugpy",
"request": "launch",
"program": "${file}",
"console": "integratedTerminal"
}
]
}
11 changes: 11 additions & 0 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"python.testing.unittestEnabled": false,
"python.testing.pytestEnabled": true,
"python.testing.autoTestDiscoverOnSaveEnabled": true,
"python.testing.promptToConfigure": true,
"python.testing.pytestArgs": ["-v", "--color=yes", "--no-cov"],
"python.testing.cwd": "./tests",
"pre-commit-helper.config": "${workspaceFolder}/.pre-commit-config.yaml",
"pre-commit-helper.runOnSave": "all hooks",
"pre-commit-helper.runOnSaveRegex": "^(.*)\\.(py|c|h)$"
}
2 changes: 1 addition & 1 deletion setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,5 @@ run-coverage = true
addopts = --cov=wxflow --cov-report=term-missing --cov-report=xml --cov-report=html

[tool:pycodestyle]
exclude = .git,.github,venv,.vscode,docs/conf.py
exclude = .git,.github,.venv,venv,.vscode,docs/conf.py
max-line-length = 160
51 changes: 43 additions & 8 deletions src/wxflow/file_utils.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
from logging import getLogger

from .fsutils import cp, mkdir
Expand All @@ -17,13 +18,20 @@

NOTE
----
"action" can be one of mkdir", "copy", etc.
"action" can be one of "mkdir", "copy", "copy_req", "copy_opt", etc.
Corresponding "act" would be ['dir1', 'dir2'], [['src1', 'dest1'], ['src2', 'dest2']]
"copy_req" will raise an error if the source file does not exist
"copy_opt" will not raise an error if the source file does not exist but will present a warning

Attributes
----------
config : dict
Dictionary of files to manipulate

NOTE
----
`copy` will be deprecated in the future in favor of `copy_req` and `copy_opt`
Users are encouraged to use `copy_req` and `copy_opt` instead of `copy`
"""

def __init__(self, config):
Expand All @@ -35,15 +43,25 @@
Method to execute bulk actions on files described in the configuration
"""
sync_factory = {
'copy': self._copy_files,
'copy': self.copy_req,
'copy_req': self.copy_req,
Copy link
Contributor

Choose a reason for hiding this comment

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

copy and copy_req seems redundant since their behavior is the same. Why not just have copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept the distinction for clarity. If it were my choice, I would deprecate copy and make it explicit copy_req so it becomes clear.
Currently, we need to keep copy since that will break existing use.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I would suggest comments to indicate that copy is moving towards deprecation and shouldn't be used in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. If that is the consensus from the team, we can make that happen.
I will make a comment that copy will be deprecated by XYZ, and users should use copy_req instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'copy_opt': self.copy_opt,
'mkdir': self._make_dirs,
}
# loop through the configuration keys
for action, files in self.config.items():
sync_factory[action](files)

@staticmethod
def _copy_files(filelist):
def copy_req(filelist):
FileHandler._copy_files(filelist, required=True)

@staticmethod
def copy_opt(filelist):
FileHandler._copy_files(filelist, required=False)

@staticmethod
def _copy_files(filelist, required=True):
"""Function to copy all files specified in the list

`filelist` should be in the form:
Expand All @@ -53,15 +71,28 @@
----------
filelist : list
List of lists of [src, dest]
required : bool, optional
Flag to indicate if the src file is required to exist. Default is True
"""
for sublist in filelist:
if len(sublist) != 2:
raise Exception(
raise IndexError(

Check warning on line 79 in src/wxflow/file_utils.py

View check run for this annotation

Codecov / codecov/patch

src/wxflow/file_utils.py#L79

Added line #L79 was not covered by tests
f"List must be of the form ['src', 'dest'], not {sublist}")
src = sublist[0]
dest = sublist[1]
cp(src, dest)
logger.info(f'Copied {src} to {dest}')
if os.path.exists(src):
try:
cp(src, dest)
logger.info(f'Copied {src} to {dest}')
except Exception as ee:
logger.exception(f"Error copying {src} to {dest}")
raise ee(f"Error copying {src} to {dest}")

Check warning on line 89 in src/wxflow/file_utils.py

View check run for this annotation

Codecov / codecov/patch

src/wxflow/file_utils.py#L87-L89

Added lines #L87 - L89 were not covered by tests
else:
if required:
logger.exception(f"Source file '{src}' does not exist and is required, ABORT!")
raise FileNotFoundError(f"Source file '{src}' does not exist")
else:
logger.warning(f"Source file '{src}' does not exist, skipping!")

@staticmethod
def _make_dirs(dirlist):
Expand All @@ -73,5 +104,9 @@
List of directories to create
"""
for dd in dirlist:
mkdir(dd)
logger.info(f'Created {dd}')
try:
mkdir(dd)
logger.info(f'Created {dd}')
except Exception as ee:
logger.exception(f"Error creating directory {dd}")
raise ee(f"Error creating directory {dd}")

Check warning on line 112 in src/wxflow/file_utils.py

View check run for this annotation

Codecov / codecov/patch

src/wxflow/file_utils.py#L110-L112

Added lines #L110 - L112 were not covered by tests
4 changes: 4 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
import os
import sys

sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), '../src')))
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this file needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It tells the package where the src is in relation to the tests. Some packages do not keep code under src/app, but directly at app/

Another use for this file is to provide test fixtures that are/could be used across tests. Eg. if test_a.py and test_c.py needs something that is common, it can be a pytest fixture defined in conftest.py.

15 changes: 15 additions & 0 deletions tests/test_file_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,18 @@ def test_copy(tmp_path):
# Check if files were indeed copied
for ff in dest_files:
assert os.path.isfile(ff)

# Create a config object for copying optional non-existent files (c.txt does not exist)
copy_list.append([input_dir_path / 'c.txt', output_dir_path / 'c.txt'])
config = {'copy_opt': copy_list}

# Copy input files to output files
FileHandler(config).sync()

# Create a config object for copying required non-existent files (c.txt does not exist)
config = {'copy_req': copy_list}
try:
FileHandler(config).sync()
except FileNotFoundError as e:
c_file = input_dir_path / 'c.txt'
assert f"Source file '{c_file}' does not exist" in str(e)