[fact] Refactor SanitizeRepo.command #139
Conversation
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 10 +2
Lines 255 285 +30
Branches 43 46 +3
=========================================
+ Hits 255 285 +30
Continue to review full report at Codecov.
|
tohanss
force-pushed
the
refactor-sanitize
branch
from
afdd90e to
9da5efc
Compare
|
Had to force push to remove an unrelated commit, no need to worry about that |
|
Based on the number of deleted lines on this PR id say sanitizer was due for refactoring, not to mention if we make a PR for SanitizeFile as well. Most added lines are just git not understanding some things that were changed to be shorter. @slarse i will make a second check tomorrow with fresh eyes, otherwise, this is ready for you to look at whenever you feel like it |
slarse
left a comment
slarse
left a comment
There was a problem hiding this comment.
Some minor comments
repobee_sanitizer/_sanitize_repo.py
Outdated
| def sanitize_files( | ||
| basedir: pathlib.Path, file_relpaths: List[_fileutils.RelativePath] | ||
| ) -> List[_format.FileWithErrors]: | ||
| ): |
There was a problem hiding this comment.
Omitting the type hint is not the same as stating that it returns None, as type hints themselves are optional. Omitting the type hint implicitly makes the type Any, which can be substituted for any type.
| ): | |
| ) -> None: |
repobee_sanitizer/_sanitize_repo.py
Outdated
| def sanitize_to_target_branch( | ||
| repo_path: pathlib.Path, target_branch: str, commit_message: str | ||
| ) -> List[_format.FileWithErrors]: | ||
| ): |
There was a problem hiding this comment.
| ): | |
| ) -> None: |
repobee_sanitizer/_sanitize_repo.py
Outdated
| _syntax, | ||
| _format, | ||
| _gitutils, | ||
| sanitizer, |
There was a problem hiding this comment.
This introduces a circular import. sanitizer imports _sanitize_repo, and vice versa. That's in general not a good sign, and can sometimes cause the program to not be able to execute at all (infinite import loop).
repobee_sanitizer/sanitizer.py
Outdated
| class SanitizeError(plug.PlugError): | ||
| def __init__(self, msg: str, cause: Optional[Exception] = None): | ||
| self.msg = msg | ||
| super().__init__(msg, cause) |
There was a problem hiding this comment.
Put this somewhere else to avoid the circular import. Also, is this something that you want to be part of the "public API" of sanitizer (even though sanitizer isn't designed as a library)?
repobee_sanitizer/sanitizer.py
Outdated
| status = plug.Status.SUCCESS | ||
| except SanitizeError as err: | ||
| result_message = err.msg | ||
| status = plug.Status.ERROR |
There was a problem hiding this comment.
Why not just return directly in the try/except? That gives two very clear, very different exits (because they are very different, one is an error exit and the other one is a success exit).
| status = plug.Status.SUCCESS | |
| except SanitizeError as err: | |
| result_message = err.msg | |
| status = plug.Status.ERROR | |
| return plug.Result(name="sanitize-repo", msg=result_message, status=plug.Status.SUCCESS) | |
| except SanitizeError as err: | |
| return plug.Result(name="sanitize-repo", msg=result_message, status=plug.Status.ERROR) |
There was a problem hiding this comment.
I felt one return statement was clearner. But when you think of them as completely different i think youre right
|
Will review this tonight. |
slarse
left a comment
slarse
left a comment
There was a problem hiding this comment.
Since this is actually my PR I can't approve it, but LGTM. I'd suggest fixing the test failures on master before merging though.
2 participants
@tohanss Here's the refactoring we looked at. I was gonna look it over again but I just didn't get around to it. Maybe you can make something of it.
Note that the core idea here was to have one form of error handling (here, we use exceptions). It would be equally valid to go with more monad-esque error handling and not throw exceptions. The important thing is to go with one kind.