fix: Bump old deps, fix tests #361
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Refactoring
PR Summary: This pull request encompasses a series of refactoring changes aimed at improving code readability, maintainability, and performance. It includes updates to the handling of hashing methods in user models, adjustments to the way solution instances are created, enhancements to rate limiting storage, and clarifications in the notifications system. Additionally, it involves code style improvements in tests and other minor refactoring across various files.
Decision: Comment
📝 Type: 'Refactoring' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
- Big diff: the diff is too large to approve with confidence.
General suggestions:
- Consider future-proofing the approach to supported hashing methods by using a more dynamic solution, such as a configuration file or database table, to facilitate easier updates.
- When directly accessing attributes of objects like
exercise.idandsolver.id, it's beneficial to use methods or properties to enhance encapsulation and maintainability. - For rate limiting storage, especially in a production or distributed environment, using in-memory storage might lead to inconsistencies. A centralized storage solution like Redis or a database is recommended.
- Maintain consistency in parameter types across the codebase to avoid ambiguity, especially when using union types like
int | User. Consider using a single type or splitting functions to handle different types more explicitly. - When dealing with path-like objects, using
os.fspath()for conversion can ensure compatibility across different Python versions and clarify the code's intention.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -247,14 +247,15 @@ def __str__(self): | |||
| @pre_save(sender=User) | |||
| def on_save_handler(model_class, instance, created): | |||
| """Hash password on creation/save.""" | |||
| supported_hashing = ('pbkdf2:sha256', 'scrypt:') | |||
There was a problem hiding this comment.
suggestion (llm): While the introduction of a tuple to check for supported hashing methods improves readability and maintainability, consider extending this approach to accommodate future hashing methods more dynamically. For instance, maintaining a configuration file or a database table for supported hashing methods could allow for easier updates without modifying the codebase.
| @@ -707,8 +706,8 @@ def create_solution( | |||
| raise AlreadyExists('This solution already exists.') | |||
|
|
|||
| instance = cls.create(**{ | |||
| cls.exercise.name: exercise, | |||
| cls.solver.name: solver, | |||
| cls.exercise.name: exercise.id, | |||
There was a problem hiding this comment.
suggestion (llm): Directly accessing the id attribute of exercise and solver objects for the creation of a solution instance introduces a tight coupling between the create_solution method and the internal structure of these objects. Consider using a method or property of these objects to obtain the necessary identifiers to enhance encapsulation and maintainability.
| limiter = Limiter( | ||
| app=webapp, | ||
| key_func=get_remote_address, | ||
| storage_uri='memory://', |
There was a problem hiding this comment.
issue (llm): Using in-memory storage for rate limiting in a production environment might not be the best choice, especially if the application is expected to run in a distributed setup. This could lead to inconsistencies in rate limiting. Consider using a centralized storage solution like Redis or a database that supports all instances of the application.
| @@ -33,7 +33,10 @@ def read(user: Optional[User] = None, id_: Optional[int] = None) -> bool: | |||
| return all(is_success) # Not gen to prevent lazy evaluation | |||
|
|
|||
|
|
|||
| def read_related(related_id: int, user: int): | |||
| def read_related(related_id: int, user: int | User): | |||
There was a problem hiding this comment.
suggestion (llm): The use of union type int | User for the user parameter in read_related function improves flexibility but also introduces ambiguity in the function's interface. It's recommended to maintain consistency in parameter types across the codebase. If the function is expected to work with both User objects and user IDs, consider splitting this into two distinct functions or consistently use one type and perform necessary conversions within the function.
| @@ -209,7 +209,7 @@ def _load_files_from_repository(self) -> typing.List[upload.File]: | |||
| """ | |||
| with tempfile.TemporaryDirectory() as tempdir: | |||
| self._execute_command( | |||
| args=['git', 'clone', self.repository_folder, '.'], | |||
| args=['git', 'clone', str(self.repository_folder), '.'], | |||
There was a problem hiding this comment.
suggestion (llm): Explicitly converting self.repository_folder to a string suggests that its type might not be inherently a string. If self.repository_folder is expected to be a path-like object, consider using os.fspath() for conversion to ensure compatibility across different Python versions and clarity in the code's intention.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #361 +/- ##
==========================================
- Coverage 84.13% 80.12% -4.01%
==========================================
Files 63 63
Lines 2949 2953 +4
==========================================
- Hits 2481 2366 -115
- Misses 468 587 +119 ☔ View full report in Codecov by Sentry. |
No description provided.