feat: add io/readers to ruff config #94
Conversation
WalkthroughThe pull request introduces modifications to several files in the Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/readii/io/loaders/images.py (2)
32-34: Consider using f-strings consistentlyWhile the error logging is good, the message construction could be more consistent with Python's modern practices.
- msg = f"Directory {raw_data_dir} does not exist." + msg = f"Directory '{raw_data_dir}' does not exist."
38-40: Enhance error messages with more contextThe error handling pattern is good, but consider adding more context to help users troubleshoot:
- For NotADirectoryError: Include the actual file type
- For FileNotFoundError: Include the full search pattern
- msg = f"Path {raw_data_dir} is not a directory." + msg = f"Path '{raw_data_dir}' is not a directory (type: {raw_data_dir.stat().st_mode})" - msg = f"No files with prefix {feature_file_prefix} and suffix {feature_file_suffix} found in directory {raw_data_dir}." + msg = f"No files matching pattern '{feature_file_prefix}*{feature_file_suffix}' found in directory '{raw_data_dir}'"Also applies to: 44-46
src/readii/io/loaders/general.py (1)
113-115: Consider adding data shape informationWhen logging empty DataFrame errors, it would be helpful to include the shape information.
- msg = "Loaded Dataframe is empty" + msg = f"Loaded DataFrame is empty (shape: {df.shape})"src/readii/io/loaders/features.py (5)
10-10: Justify or remove the noqa comment.The
# noqacomment is suppressing linter warnings, but it's not clear which specific warning is being suppressed. Consider either:
- Specifying the exact warning code being suppressed (e.g.,
# noqa: E501for line length)- Removing it if no longer needed
Line range hint
14-32: Update docstring to match the new type annotation.The parameter type for
extracted_feature_dirshould be updated to reflect that it accepts both string and Path objects:Parameters ---------- - extracted_feature_dir : str + extracted_feature_dir : Union[str, Path] Path to the directory containing the extracted feature csv files
45-48: Avoid duplicating the error message.The error message is defined but not used in the exception. Consider:
if not extracted_feature_dir.exists() or not extracted_feature_dir.is_dir(): msg = f"Extracted feature directory {extracted_feature_dir} does not exist." - logger.error(f"Extracted feature directory {extracted_feature_dir} does not exist.") - raise FileNotFoundError() + logger.error(msg) + raise FileNotFoundError(msg)
76-78: Consider more specific exception handling.The current implementation catches all exceptions with a broad
Exceptionclause. Consider catching specific exceptions that might occur during file operations:- except Exception as e: + except (OSError, IOError) as e: logger.warning(f"Error loading {image_type} feature csv files from {extracted_feature_dir}: {e}") raise e
104-105: Include error message in ValueError.Consider passing the error message to the ValueError for better debugging:
logger.error(f"No valid feature sets were loaded from {extracted_feature_dir}") - raise ValueError() + raise ValueError(f"No valid feature sets were loaded from {extracted_feature_dir}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config/ruff.toml(1 hunks)src/readii/io/loaders/features.py(4 hunks)src/readii/io/loaders/general.py(4 hunks)src/readii/io/loaders/images.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/readii/io/loaders/features.py
93-93: SyntaxError: Expected an indented block after except clause
🔇 Additional comments (7)
src/readii/io/loaders/images.py (2)
4-5: LGTM: Logger import added correctly
The logger import from readii.utils is appropriately placed at the module level.
9-9: LGTM: Return type annotation added
The return type annotation -> list improves type safety and documentation.
src/readii/io/loaders/general.py (2)
6-7: LGTM: Logger import added correctly
The logger import from readii.utils is appropriately placed at the module level.
45-46: LGTM: Error handling in loadImageDatasetConfig
The error handling pattern is consistent and appropriate:
- FileNotFoundError for missing config
- ConfigError for YAML parsing issues
- ConfigError for empty config
Also applies to: 52-54, 57-59
config/ruff.toml (2)
Line range hint 1-116: LGTM: Comprehensive ruff configuration
The configuration is well-structured with:
- Clear categorization of rules (type annotations, imports, code quality, etc.)
- Appropriate ignores for specific cases
- Consistent formatting settings
9-9: Verify the impact of broader include pattern
The pattern src/readii/io/**/**.py will include all Python files under io directory. Let's verify this doesn't accidentally include any test files or other unintended files.
✅ Verification successful
The broader include pattern correctly targets only source files
The pattern src/readii/io/**/**.py is safe to use as it only includes legitimate source files:
- All files are properly organized under feature-specific subdirectories (loaders, utils, writers)
- No test files were found in the io directory structure
- The included files align with expected source code organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# List all Python files that will be included by the new pattern
fd '.py$' src/readii/io/
# Check if there are any test files that might be accidentally included
fd -I 'test.*\.py$' src/readii/io/
Length of output: 440
src/readii/io/loaders/features.py (1)
33-36: LGTM! Clean Path object handling.
The conversion from string to Path is implemented correctly with proper type checking.
| msg = "Error pargins {file_path}" | ||
| logger.exception(msg) | ||
| raise DataFrameLoadError() from e |
There was a problem hiding this comment.
Fix typo in error message
There's a typo in the error message for file parsing errors.
- msg = "Error pargins {file_path}"
+ msg = f"Error parsing '{file_path}'"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| msg = "Error pargins {file_path}" | |
| logger.exception(msg) | |
| raise DataFrameLoadError() from e | |
| msg = f"Error parsing '{file_path}'" | |
| logger.exception(msg) | |
| raise DataFrameLoadError() from e |
src/readii/io/loaders/features.py
Outdated
| except KeyError: | ||
| except KeyError: |
There was a problem hiding this comment.
Fix the duplicated except clause.
There's a syntax error with the duplicated except KeyError: line:
- except KeyError:
except KeyError:
logger.warning(f"{feature_file_path} does not have the labels {labels_to_drop} to drop.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except KeyError: | |
| except KeyError: | |
| except KeyError: | |
| logger.warning(f"{feature_file_path} does not have the labels {labels_to_drop} to drop.") |
🧰 Tools
🪛 Ruff (0.8.2)
93-93: SyntaxError: Expected an indented block after except clause
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/readii/io/loaders/features.py (4)
10-10: Remove unnecessary noqa comment.The
# noqacomment appears unnecessary as there are no apparent linting issues with this line. Consider removing it unless there's a specific linting rule being suppressed.
Line range hint
14-32: Update docstring to reflect new parameter type.The docstring for
extracted_feature_dirparameter should be updated to reflect that it now accepts both string and Path objects.- extracted_feature_dir : str + extracted_feature_dir : Union[str, Path]
45-48: Improve error handling consistency.The error handling can be improved:
- The
msgvariable is defined but not used in the exception- The error logging message is duplicated
if not extracted_feature_dir.exists() or not extracted_feature_dir.is_dir(): msg = f"Extracted feature directory {extracted_feature_dir} does not exist." - logger.error(f"Extracted feature directory {extracted_feature_dir} does not exist.") + logger.error(msg) - raise FileNotFoundError() + raise FileNotFoundError(msg)
104-105: Include error message in ValueError.The ValueError should include the error message for better debugging.
logger.error(f"No valid feature sets were loaded from {extracted_feature_dir}") - raise ValueError() + raise ValueError(f"No valid feature sets were loaded from {extracted_feature_dir}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pixi.lockis excluded by!**/*.lock
📒 Files selected for processing (1)
src/readii/io/loaders/features.py(4 hunks)
🔇 Additional comments (1)
src/readii/io/loaders/features.py (1)
10-13: Verify all callers handle the new parameter type.
The function signature has been updated to accept both Path and str types. Let's verify that all callers of this function are compatible with this change.
✅ Verification successful
Function appears to be newly added or currently unused
The function loadFeatureFilesFromImageTypes is only defined in src/readii/io/loaders/features.py and exported in src/readii/io/loaders/__init__.py, but there are no actual calls to this function found in the codebase. This suggests that either:
- The function is newly added and will be used in future implementations
- The function is used in external code that imports this package
- The function is used in test files that might be in a separate directory
Since the change to accept both Path and str types makes the function more flexible for future use, and there are no current callers that need updating, this change is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to loadFeatureFilesFromImageTypes
ast-grep --pattern 'loadFeatureFilesFromImageTypes($$$)'
Length of output: 58
Script:
#!/bin/bash
# Search for function calls using ripgrep for better coverage
rg "loadFeatureFilesFromImageTypes" -l
# Also search for the function definition to understand its location
rg "def loadFeatureFilesFromImageTypes" -l
Length of output: 185
Script:
#!/bin/bash
# Let's see the actual usage context in both files
rg "loadFeatureFilesFromImageTypes" -B 2 -A 2 src/readii/io/loaders/features.py src/readii/io/loaders/__init__.py
Length of output: 1177
Script:
#!/bin/bash
# Let's search for any imports and actual usage of this function in the entire codebase
rg -g '!{features,__init__}.py' "loadFeatureFilesFromImageTypes" -B 3 -A 3
Length of output: 74
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
- Coverage 49.04% 40.31% -8.73%
==========================================
Files 25 32 +7
Lines 991 1399 +408
==========================================
+ Hits 486 564 +78
- Misses 505 835 +330 ☔ View full report in Codecov by Sentry. |
| continue | ||
| case _: | ||
| # Multiple files found | ||
| msg = f"Multiple {image_type} feature csv files found in {extracted_feature_dir}. First one will be used." |
There was a problem hiding this comment.
| msg = f"Multiple {image_type} feature csv files found in {extracted_feature_dir}. First one will be used." | |
| msg = f"{len(matching_files) csv files found for {image_type} in {extracted_feature_dir}. First one ({matching_files[0]) will be used." |
| except Exception as e: | ||
| logger.warning(f"Error loading {image_type} feature csv files from {extracted_feature_dir}: {e}") | ||
| raise e |
There was a problem hiding this comment.
what kind of exceptions do you forsee in this try-except block? Looks like the most possible source of errors are well handled i.e no chance for index error
Updated include list to have the io/readers functions and updated the files so all ruff tests pass.
Summary by CodeRabbit
New Features
Pathobjects and strings.Bug Fixes
Documentation