GeezerVison is showing github.com — Draft: feat: Add column to DB + Migration by yammesicka · Pull Request #377 · PythonFreeCourse/lms
Conversation
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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 @yammesicka - I've reviewed your changes and they look great!
General suggestions:
- Consider aligning the naming of the new column across the model and migration function for consistency and clarity.
- Review the return value of the migration function to ensure it provides meaningful feedback about the migration's success.
- Explore abstracting column addition into a more generic function to simplify future migrations and adhere to DRY principles.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
sourcery-ai[bot] reacted with thumbs up emoji
sourcery-ai[bot] reacted with thumbs down emoji
All reactions
-
👍
1 reaction
-
👎
1 reaction
|
|
exercise = ForeignKeyField(Exercise, backref='solutions')
|
|
|
solver = ForeignKeyField(User, backref='solutions')
|
|
|
checker = ForeignKeyField(User, null=True, backref='solutions')
|
|
|
is_key_answer = BooleanField(default=False)
|
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_clarification): Consider renaming is_key_answer to is_model_solution for consistency.
The new column added to the Solution model is referred to as is_model_solution in the migration function but is named is_key_answer here. Aligning these names would improve code readability and maintainability.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
sourcery-ai[bot] reacted with thumbs up emoji
sourcery-ai[bot] reacted with thumbs down emoji
All reactions
-
👍
1 reaction
-
👎
1 reaction
Comment on lines
+341
to
+344
|
|
def _add_is_model_solution_column() -> bool:
|
|
|
Solution = models.Solution
|
|
|
_migrate_column_in_table_if_needed(Solution, Solution.is_model_solution)
|
|
|
return True
|
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_refinement): Ensure the return value of _add_is_model_solution_column is meaningful.
The function always returns True, which might not be informative. Consider returning a value that reflects whether the migration was necessary or successful.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
sourcery-ai[bot] reacted with thumbs up emoji
sourcery-ai[bot] reacted with thumbs down emoji
All reactions
-
👍
1 reaction
-
👎
1 reaction
|
|
log.info(f'{new_mail_address} already exists in User')
|
|
|
|
|
|
|
|
|
def _add_is_model_solution_column() -> bool:
|
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 (complexity): Consider abstracting the column addition into a more generic function.
While the added functionality in _add_is_model_solution_column achieves the intended migration task, it introduces a higher level of complexity and specificity to the migration module. This approach could potentially make the codebase harder to maintain, especially as more migrations are added over time. To streamline the migration process and adhere to the DRY principle, consider abstracting the column addition into a more generic function. For instance:
def _migrate_add_column(model, column_name, column_type):
if not column_exists(model, column_name):
with models.database.atomic():
migrator = MySQLMigrator(models.database)
migrate(
migrator.add_column(model._meta.table_name, column_name, column_type())
)
log.info(f"Column {column_name} added to {model._meta.table_name}")
else:
log.info(f"Column {column_name} already exists in {model._meta.table_name}")
This method can then be used for adding any column to any model, reducing the need for specific functions for each migration task. It simplifies the migration process, making the codebase easier to maintain and extend in the future.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
sourcery-ai[bot] reacted with thumbs up emoji
sourcery-ai[bot] reacted with thumbs down emoji
All reactions
-
👍
1 reaction
-
👎
1 reaction
codecov
Bot
commented
Mar 24, 2024
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.39%. Comparing base (d9264ad) to head (315f1c2).
⚠️ Report is 19 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #377 +/- ##
==========================================
- Coverage 80.48% 80.39% -0.10%
==========================================
Files 65 65
Lines 3049 3055 +6
==========================================
+ Hits 2454 2456 +2
- Misses 595 599 +4
☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:
- ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|