-
Notifications
You must be signed in to change notification settings - Fork 1
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
Config lefthook #25
Config lefthook #25
Conversation
Reviewer's Guide by SourceryThis pull request introduces significant changes to the project structure and functionality, focusing on improving data validation, error handling, and code organization. The changes include the addition of new models for WRROC, TES, and WES data structures, implementation of validators, updates to existing converters, and the introduction of a pre-push hook for code linting. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SalihuDickson - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
crategen/converters/tes_converter.py
Outdated
volumes, | ||
logs, | ||
tags, | ||
) = validated_tes_data.dict().values() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Use a more robust method for extracting validated data
Using .dict().values() is potentially fragile if the order of fields in the Pydantic model changes. Consider using named attributes instead, e.g., validated_tes_data.id, validated_tes_data.name, etc. This would make the code more robust and easier to understand.
) = validated_tes_data.dict().values() | |
id = validated_tes_data.id | |
name = validated_tes_data.name | |
description = validated_tes_data.description | |
executors = validated_tes_data.executors | |
inputs = validated_tes_data.inputs | |
outputs = validated_tes_data.outputs | |
volumes = validated_tes_data.volumes | |
logs = validated_tes_data.logs | |
tags = validated_tes_data.tags |
tests/unit/test_wrroc_models.py
Outdated
def test_validate_wrroc_tes_missing_fields(self): | ||
""" | ||
Test that validate_wrroc_tes raises a ValueError if required fields for TES conversion are missing. | ||
""" | ||
data = {"id": "process-id", "name": "Test Process"} | ||
with self.assertRaises(ValueError): | ||
validate_wrroc_tes(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Consider adding more specific test cases for TES validation
While this test checks for missing fields, it might be beneficial to add more specific test cases that check each required field individually. This could help pinpoint exactly which field validations might fail in the future.
def test_validate_wrroc_tes_missing_fields(self): | |
""" | |
Test that validate_wrroc_tes raises a ValueError if required fields for TES conversion are missing. | |
""" | |
data = {"id": "process-id", "name": "Test Process"} | |
with self.assertRaises(ValueError): | |
validate_wrroc_tes(data) | |
def test_validate_wrroc_tes_missing_fields(self): | |
"""Test validate_wrroc_tes raises ValueError for missing required fields.""" | |
required_fields = ['id', 'name', 'description', 'executors'] | |
for field in required_fields: | |
data = {f: "value" for f in required_fields if f != field} | |
with self.subTest(f"Missing {field}"): | |
with self.assertRaises(ValueError): | |
validate_wrroc_tes(data) |
logs, | ||
tags, | ||
) = validated_tes_data.dict().values() | ||
end_time = validated_tes_data.logs[0].end_time | ||
|
||
# Convert to WRROC | ||
wrroc_data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable
)
crategen/converters/tes_converter.py
Outdated
start_time = wrroc_data.get("startTime", "") | ||
end_time = wrroc_data.get("endTime", "") | ||
def convert_from_wrroc(self, data): | ||
# Validate WRROC data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): We've found these issues:
- Inline variable that is immediately returned (
inline-immediately-returned-variable
) - Don't assign to builtin variable
id
(avoid-builtin-shadow
)
Explanation
Python has a number of
builtin
variables: functions and constants thatform a part of the language, such as
list
, getattr
, and type
(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]
However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers
.
In a pinch, my_list
and similar names are colloquially-recognized
placeholders.
crategen/converters/wes_converter.py
Outdated
wes_data = { | ||
"run_id": run_id, | ||
"run_log": { | ||
"name": name, | ||
"start_time": start_time, | ||
"end_time": end_time, | ||
}, | ||
"run_log": {"name": name, "start_time": start_time, "end_time": end_time}, | ||
"state": state, | ||
"outputs": [{"location": res.get("@id", ""), "name": res.get("name", "")} for res in result_data], | ||
"outputs": [{"location": res.id, "name": res.name} for res in result_data], | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable
)
|
||
if content_is_set: | ||
values["url"] = None | ||
elif not content_is_set and not url_is_set: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Remove redundant conditional (remove-redundant-if
)
elif not content_is_set and not url_is_set: | |
elif not url_is_set: |
crategen/validators.py
Outdated
missing_fields = [ | ||
field for field in required_fields if getattr(validated_data, field) is None | ||
] | ||
|
||
if missing_fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression
)
missing_fields = [ | |
field for field in required_fields if getattr(validated_data, field) is None | |
] | |
if missing_fields: | |
if missing_fields := [ | |
field | |
for field in required_fields | |
if getattr(validated_data, field) is None | |
]: |
crategen/validators.py
Outdated
missing_fields = [ | ||
field for field in required_fields if getattr(validated_data, field) is None | ||
] | ||
|
||
if missing_fields: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression
)
missing_fields = [ | |
field for field in required_fields if getattr(validated_data, field) is None | |
] | |
if missing_fields: | |
if missing_fields := [ | |
field | |
for field in required_fields | |
if getattr(validated_data, field) is None | |
]: |
Summary by Sourcery
Refactor the TES and WES converters to use Pydantic for data validation, improving data integrity and error handling. Introduce new Pydantic models for TES and WES data. Enhance the CI workflow to run tests conditionally and trigger on all branches. Add unit tests for WRROC models and validators. Configure pre-push hooks using Lefthook for code linting with Ruff.
New Features:
lefthook.yml
configuration file to set up pre-push hooks for code linting with Ruff.Enhancements:
CI:
Tests: