Skip to content

[fact] Refactor SanitizeRepo.command #139

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.

Sign up for GitHub

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

Open
slarse wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[fact] Refactor SanitizeRepo.command #139

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9b6e85
Refactor SanitizeRepo.command
slarse Jul 19, 2021
9da5efc
Modify command function to only have one return
tohanss Jul 23, 2021
b19dfee
Refactor some more
tohanss Jul 23, 2021
2cfdabd
Implement feedback
tohanss Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions repobee_sanitizer/_gitutils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,7 @@ def create_pr_branch(repo_path: pathlib.Path, target_branch: str) -> str:

def branch_exists(repo_path: pathlib.Path, target_branch: str) -> bool:
repo = git.Repo(str(repo_path))

existing_branches = [branch.name for branch in repo.branches]

return target_branch in existing_branches
return target_branch in [branch.name for branch in repo.branches]


def _git_commit_on_branch(
Expand Down
26 changes: 11 additions & 15 deletions repobee_sanitizer/_sanitize_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@
_syntax,
_format,
_gitutils,
sanitizer,
Copy link
Contributor Author

@slarse slarse Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

)

PLUGIN_NAME = "sanitizer"

LOGGER = daiquiri.getLogger(__file__)


def check_repo_state(repo_root) -> Optional[str]:
def check_repo_state(repo_root, ignore_commit_errors) -> Optional[str]:
try:
repo = git.Repo(repo_root)
except git.InvalidGitRepositoryError as exc:
Expand All @@ -42,7 +43,8 @@ def check_repo_state(repo_root) -> Optional[str]:
if repo.index.diff(None):
message = "There are uncommitted unstaged files in the repo"

return message + help_message if message else None
if message and not ignore_commit_errors:
raise sanitizer.SanitizeError(msg=message + help_message)


def discover_dirty_files(
Expand All @@ -64,7 +66,7 @@ def discover_dirty_files(

def sanitize_files(
basedir: pathlib.Path, file_relpaths: List[_fileutils.RelativePath]
) -> List[_format.FileWithErrors]:
):
Copy link
Contributor Author

@slarse slarse Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
):
) -> None:

"""Checks the syntax of the provided files and reports any found errors.
If any errors are found, report errors and exits. If there are no errors,
then all files are sanitized."""
Expand All @@ -77,7 +79,9 @@ def sanitize_files(
files_with_errors.append(_format.FileWithErrors(relpath, errors))

if files_with_errors:
return files_with_errors
raise sanitizer.SanitizeError(
msg=_format.format_error_string(files_with_errors)
)

for relpath in file_relpaths:
content = relpath.read_text_relative_to(basedir).split("\n")
Expand All @@ -91,12 +95,10 @@ def sanitize_files(
)
LOGGER.info(f"Sanitized {relpath}")

return []


def sanitize_to_target_branch(
repo_path: pathlib.Path, target_branch: str, commit_message: str
) -> List[_format.FileWithErrors]:
):
Copy link
Contributor Author

@slarse slarse Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
):
) -> None:

"""Create a commit on the target branch of the specified repo with
sanitized versions of the provided files, without modifying the
working tree or HEAD of the repo.
Expand All @@ -106,19 +108,15 @@ def sanitize_to_target_branch(
file_relpaths: A list of paths relative to the root of the working
tree that should be sanitized.
target_branch: The branch to create the commit on.

Returns:
List of errors if any errors are found, otherwise an empty list.
"""

with tempfile.TemporaryDirectory() as tmpdir:
repo_copy_path = pathlib.Path(tmpdir) / "repo"
shutil.copytree(src=repo_path, dst=repo_copy_path)
_gitutils._clean_repo(repo_copy_path)
file_relpaths = discover_dirty_files(repo_copy_path)
errors = sanitize_files(repo_copy_path, file_relpaths)
if errors:
return errors
sanitize_files(repo_copy_path, file_relpaths)

_gitutils._git_commit_on_branch(
repo_copy_path, target_branch, commit_message
)
Expand All @@ -128,5 +126,3 @@ def sanitize_to_target_branch(
dst_repo_path=repo_path,
dst_branch=target_branch,
)

return []
107 changes: 53 additions & 54 deletions repobee_sanitizer/sanitizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,13 @@
PLUGIN_NAME = "sanitizer"
LOGGER = daiquiri.getLogger(__file__)


class SanitizeError(plug.PlugError):
def __init__(self, msg: str, cause: Optional[Exception] = None):
self.msg = msg
super().__init__(msg, cause)
Copy link
Contributor Author

@slarse slarse Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?



sanitize_category = plug.cli.category(
"sanitize",
action_names=["repo", "file"],
Expand All @@ -45,8 +52,8 @@ class SanitizeRepo(plug.Plugin, plug.cli.Command):
repo_root = plug.cli.option(
short_name="-r",
help="manually provide path to the root of git repository",
converter=pathlib.Path,
default=pathlib.Path("."),
converter=lambda s: pathlib.Path(s).resolve(strict=True),
default=".",
)
commit_message = plug.cli.option(
short_name="-m", default="Update task template", configurable=True
Expand All @@ -72,79 +79,71 @@ class SanitizeRepo(plug.Plugin, plug.cli.Command):
)

def command(self) -> Optional[plug.Result]:
repo_root = self.repo_root.absolute()

input_error = self._validate_input(repo_root)
if input_error:
return input_error

if self.no_commit:
LOGGER.info("Executing dry run")
file_relpaths = _sanitize_repo.discover_dirty_files(repo_root)
errors = _sanitize_repo.sanitize_files(repo_root, file_relpaths)
else:
LOGGER.info(f"Sanitizing repo and updating {self.target_branch}")
effective_target_branch = self._resolve_effective_target_branch(
repo_root
try:
self._validate_input()
result_message = (
self._sanitize_no_commit()
if self.no_commit
else self._sanitize_to_target_branch()
)
try:
errors = _sanitize_repo.sanitize_to_target_branch(
repo_root, effective_target_branch, self.commit_message
)
except _gitutils.EmptyCommitError:
return plug.Result(
name="sanitize-repo",
msg="No diff between target branch and sanitized output. "
f"No changes will be made to branch: {self.target_branch}",
status=plug.Status.WARNING,
)
status = plug.Status.SUCCESS
except SanitizeError as err:
result_message = err.msg
status = plug.Status.ERROR
Copy link
Contributor Author

@slarse slarse Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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)

Copy link
Contributor

@tohanss tohanss Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt one return statement was clearner. But when you think of them as completely different i think youre right


if errors:
return plug.Result(
name="sanitize-repo",
msg=_format.format_error_string(errors),
status=plug.Status.ERROR,
return plug.Result(
name="sanitize-repo", msg=result_message, status=status
)

def _sanitize_no_commit(self) -> str:
LOGGER.info("Executing dry run")
file_relpaths = _sanitize_repo.discover_dirty_files(self.repo_root)
_sanitize_repo.sanitize_files(self.repo_root, file_relpaths)

return "Successfully sanitized repo"

def _sanitize_to_target_branch(self) -> str:
LOGGER.info(f"Sanitizing repo and updating {self.target_branch}")
effective_target_branch = self._resolve_effective_target_branch(
self.repo_root
)
try:
_sanitize_repo.sanitize_to_target_branch(
self.repo_root, effective_target_branch, self.commit_message
)
except _gitutils.EmptyCommitError:
return (
"No diff between target branch and sanitized output. "
f"No changes will be made to branch: {self.target_branch}"
)

result_message = "Successfully sanitized repo" + (
return "Successfully sanitized repo" + (
f" to pull request branch\n\nrun 'git switch "
f"{effective_target_branch}' to checkout the branch"
if self.create_pr_branch
else ""
)

return plug.Result(
name="sanitize-repo",
msg=result_message,
status=plug.Status.SUCCESS,
)

def _resolve_effective_target_branch(self, repo_root: pathlib.Path) -> str:
if self.create_pr_branch:
return _gitutils.create_pr_branch(repo_root, self.target_branch)
return self.target_branch

def _validate_input(self, repo_root) -> plug.Result:
message = _sanitize_repo.check_repo_state(repo_root)
if message and not self.force:
return plug.Result(
name="sanitize-repo", msg=message, status=plug.Status.ERROR
)
def _validate_input(self) -> plug.Result:
_sanitize_repo.check_repo_state(self.repo_root, self.force)

if self.create_pr_branch:
if not self.target_branch:
return plug.Result(
name="sanitize-repo",
raise SanitizeError(
msg="Can not create a pull request without a target "
"branch, please specify --target-branch",
status=plug.Status.ERROR,
"branch, please specify --target-branch"
)
elif not _gitutils.branch_exists(repo_root, self.target_branch):
return plug.Result(
name="sanitize-repo",
elif not _gitutils.branch_exists(
self.repo_root, self.target_branch
):
raise SanitizeError(
msg=f"Can not create a pull request branch from "
f"non-existing target branch {self.target_branch}",
status=plug.Status.ERROR,
f"non-existing target branch {self.target_branch}"
)


Expand Down