-
Notifications
You must be signed in to change notification settings - Fork 218
Patch patch command #3714
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
Open
ErikDanielsson
wants to merge
6
commits into
nf-core:dev
Choose a base branch
from
ErikDanielsson:patch-patch-command
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Patch patch command #3714
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
8adcbca
Add fail to decode cases to enum. Improve output
ErikDanielsson 9080a60
Add questionary prompt to handle files that we failed to read
ErikDanielsson 423daac
Move prompt to diff creator
ErikDanielsson e76e32e
Add ignore list
ErikDanielsson 0067542
[automated] Update CHANGELOG.md
nf-core-bot 548cf05
Apply suggestions from code review
ErikDanielsson File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ | |
| import json | ||
| import logging | ||
| import os | ||
| from collections.abc import Iterator | ||
| from pathlib import Path | ||
| from typing import Union | ||
| from typing import Optional, Union | ||
|
|
||
| import questionary | ||
| from rich import box | ||
| from rich.console import Console, Group, RenderableType | ||
| from rich.panel import Panel | ||
|
|
@@ -32,9 +34,11 @@ class DiffEnum(enum.Enum): | |
| CHANGED = enum.auto() | ||
| CREATED = enum.auto() | ||
| REMOVED = enum.auto() | ||
| FROM_DECODE_FAIL = enum.auto() | ||
| TO_DECODE_FAIL = enum.auto() | ||
|
|
||
| @staticmethod | ||
| def get_component_diffs(from_dir, to_dir, for_git=True, dsp_from_dir=None, dsp_to_dir=None): | ||
| def get_component_diffs(from_dir, to_dir, for_git=True, dsp_from_dir=None, dsp_to_dir=None, ignore_files={}): | ||
| """ | ||
| Compute the diff between the current component version | ||
| and the new version. | ||
|
|
@@ -71,57 +75,127 @@ def get_component_diffs(from_dir, to_dir, for_git=True, dsp_from_dir=None, dsp_t | |
| ) | ||
| ) | ||
| files = list(files) | ||
| log.info(files) | ||
|
|
||
| # Loop through all the component files and compute their diffs if needed | ||
| for file in files: | ||
| temp_path = Path(to_dir, file) | ||
| curr_path = Path(from_dir, file) | ||
| if temp_path.exists() and curr_path.exists() and temp_path.is_file(): | ||
| diff = ComponentsDiffer.get_file_diff( | ||
| file=file, | ||
| to_dir=to_dir, | ||
| from_dir=from_dir, | ||
| dsp_to_dir=dsp_to_dir, | ||
| dsp_from_dir=dsp_from_dir, | ||
| ignore_files=ignore_files, | ||
| ) | ||
| if diff is not None: | ||
| diffs[file] = diff | ||
|
|
||
| return diffs | ||
|
|
||
| @staticmethod | ||
| def handle_decode_error(file, base_dir, type: DiffEnum, e: UnicodeDecodeError, ignore_files: dict[Path, bool]): | ||
| if file in ignore_files: | ||
| # We have already seen this file, so do not prompt again | ||
| ans = "i" if ignore_files[file] else "w" | ||
| else: | ||
| log.warning( | ||
| f"We failed to read the file: [bold red]{file}[/]. Please check that the component's directory contains only text files." | ||
| ) | ||
| ans = questionary.select( | ||
| "Do you still want to proceed with constructing the diff?", | ||
| choices=[ | ||
| questionary.Choice("Abort diff creation", "a"), | ||
| questionary.Choice("Ignore file", "i"), | ||
| questionary.Choice("Warn in diff", "w"), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to offer the option to warn, if we can't read the file, I would say either we abort and let the user remove it, or ignore the file. |
||
| ], | ||
| style=nf_core.utils.nfcore_question_style, | ||
| ).unsafe_ask() | ||
|
|
||
| if ans == "w": | ||
| ignore_files[file] = False | ||
| return type, iter(()) | ||
| elif ans == "a": | ||
| raise OSError from e | ||
| elif ans == "i": | ||
| ignore_files[file] = True | ||
| return None | ||
| else: | ||
| raise UserWarning("This should never occur") | ||
|
|
||
| @staticmethod | ||
| def get_file_diff( | ||
| file, to_dir, from_dir, dsp_to_dir, dsp_from_dir, ignore_files=[] | ||
| ) -> Optional[tuple[DiffEnum, Iterator[str]]]: | ||
| temp_path = Path(to_dir, file) | ||
| curr_path = Path(from_dir, file) | ||
| if temp_path.exists() and curr_path.exists() and temp_path.is_file(): | ||
| try: | ||
| with open(temp_path) as fh: | ||
| new_lines = fh.readlines() | ||
| except UnicodeDecodeError as e: | ||
| return ComponentsDiffer.handle_decode_error( | ||
| file, dsp_to_dir, ComponentsDiffer.DiffEnum.TO_DECODE_FAIL, e, ignore_files | ||
| ) | ||
|
|
||
| try: | ||
| with open(curr_path) as fh: | ||
| old_lines = fh.readlines() | ||
| except UnicodeDecodeError as e: | ||
| return ComponentsDiffer.handle_decode_error( | ||
| file, dsp_from_dir, ComponentsDiffer.DiffEnum.FROM_DECODE_FAIL, e, ignore_files | ||
| ) | ||
|
|
||
| if new_lines == old_lines: | ||
| # The files are identical | ||
| diffs[file] = (ComponentsDiffer.DiffEnum.UNCHANGED, ()) | ||
| else: | ||
| # Compute the diff | ||
| diff = difflib.unified_diff( | ||
| old_lines, | ||
| new_lines, | ||
| fromfile=str(Path(dsp_from_dir, file)), | ||
| tofile=str(Path(dsp_to_dir, file)), | ||
| ) | ||
| diffs[file] = (ComponentsDiffer.DiffEnum.CHANGED, diff) | ||
|
|
||
| elif temp_path.exists(): | ||
| with open(temp_path) as fh: | ||
| new_lines = fh.readlines() | ||
| # The file was created | ||
| # Show file against /dev/null | ||
| if new_lines == old_lines: | ||
| # The files are identical | ||
| return ComponentsDiffer.DiffEnum.UNCHANGED, iter(()) | ||
| else: | ||
| # Compute the diff | ||
| diff = difflib.unified_diff( | ||
| [], | ||
| old_lines, | ||
| new_lines, | ||
| fromfile=str(Path("/dev", "null")), | ||
| fromfile=str(Path(dsp_from_dir, file)), | ||
| tofile=str(Path(dsp_to_dir, file)), | ||
| ) | ||
| diffs[file] = (ComponentsDiffer.DiffEnum.CREATED, diff) | ||
| return ComponentsDiffer.DiffEnum.CHANGED, diff | ||
|
|
||
| elif curr_path.exists(): | ||
| # The file was removed | ||
| # Show file against /dev/null | ||
| elif temp_path.exists(): | ||
| try: | ||
| with open(temp_path) as fh: | ||
| new_lines = fh.readlines() | ||
| except UnicodeDecodeError as e: | ||
| return ComponentsDiffer.handle_decode_error( | ||
| file, dsp_to_dir, ComponentsDiffer.DiffEnum.TO_DECODE_FAIL, e, ignore_files | ||
| ) | ||
|
|
||
| # The file was created | ||
| # Show file against /dev/null | ||
| diff = difflib.unified_diff( | ||
| [], | ||
| new_lines, | ||
| fromfile=str(Path("/dev", "null")), | ||
| tofile=str(Path(dsp_to_dir, file)), | ||
| ) | ||
| return ComponentsDiffer.DiffEnum.CREATED, diff | ||
| elif curr_path.exists(): | ||
| # The file was removed | ||
| # Show file against /dev/null | ||
| try: | ||
| with open(curr_path) as fh: | ||
| old_lines = fh.readlines() | ||
| diff = difflib.unified_diff( | ||
| old_lines, | ||
| [], | ||
| fromfile=str(Path(dsp_from_dir, file)), | ||
| tofile=str(Path("/dev", "null")), | ||
| except UnicodeDecodeError as e: | ||
| return ComponentsDiffer.handle_decode_error( | ||
| file, dsp_from_dir, ComponentsDiffer.DiffEnum.FROM_DECODE_FAIL, e, ignore_files | ||
| ) | ||
| diffs[file] = (ComponentsDiffer.DiffEnum.REMOVED, diff) | ||
|
|
||
| return diffs | ||
| diff = difflib.unified_diff( | ||
| old_lines, | ||
| [], | ||
| fromfile=str(Path(dsp_from_dir, file)), | ||
| tofile=str(Path("/dev", "null")), | ||
| ) | ||
| return ComponentsDiffer.DiffEnum.REMOVED, diff | ||
| else: | ||
| raise LookupError(f"The file {file} is missing both before and after the patch") | ||
|
|
||
| @staticmethod | ||
| def write_diff_file( | ||
|
|
@@ -137,6 +211,7 @@ def write_diff_file( | |
| dsp_from_dir=None, | ||
| dsp_to_dir=None, | ||
| limit_output=False, | ||
| ignore_files={}, | ||
| ): | ||
| """ | ||
| Writes the diffs of a component to the diff file. | ||
|
|
@@ -164,7 +239,9 @@ def write_diff_file( | |
| if dsp_to_dir is None: | ||
| dsp_to_dir = to_dir | ||
|
|
||
| diffs = ComponentsDiffer.get_component_diffs(from_dir, to_dir, for_git, dsp_from_dir, dsp_to_dir) | ||
| diffs = ComponentsDiffer.get_component_diffs( | ||
| from_dir, to_dir, for_git, dsp_from_dir, dsp_to_dir, ignore_files=ignore_files | ||
| ) | ||
| if all(diff_status == ComponentsDiffer.DiffEnum.UNCHANGED for _, (diff_status, _) in diffs.items()): | ||
| raise UserWarning("Component is unchanged") | ||
| log.debug(f"Writing diff of '{component}' to '{diff_path}'") | ||
|
|
@@ -188,6 +265,14 @@ def write_diff_file( | |
| elif diff_status == ComponentsDiffer.DiffEnum.REMOVED: | ||
| # The file was removed between the commits | ||
| fh.write(f"'{Path(dsp_from_dir, file)}' was removed\n") | ||
| elif diff_status == ComponentsDiffer.DiffEnum.TO_DECODE_FAIL: | ||
| # We failed to decode the file | ||
| fh.write(f"'{Path(dsp_from_dir, file)}' failed to be decoded\n") | ||
|
|
||
| elif diff_status == ComponentsDiffer.DiffEnum.FROM_DECODE_FAIL: | ||
| # We failed to decode the file | ||
| fh.write(f"'{Path(dsp_from_dir, file)}' failed to be decoded\n") | ||
|
|
||
| elif limit_output and not file.suffix == ".nf": | ||
| # Skip printing the diff for files other than main.nf | ||
| fh.write(f"Changes in '{Path(component, file)}' but not shown\n") | ||
|
|
@@ -245,6 +330,7 @@ def print_diff( | |
| dsp_from_dir=None, | ||
| dsp_to_dir=None, | ||
| limit_output=False, | ||
| ignore_files={}, | ||
| ): | ||
| """ | ||
| Prints the diffs between two component versions to the terminal | ||
|
|
@@ -266,7 +352,7 @@ def print_diff( | |
| dsp_to_dir = to_dir | ||
|
|
||
| diffs = ComponentsDiffer.get_component_diffs( | ||
| from_dir, to_dir, for_git=False, dsp_from_dir=dsp_from_dir, dsp_to_dir=dsp_to_dir | ||
| from_dir, to_dir, for_git=False, dsp_from_dir=dsp_from_dir, dsp_to_dir=dsp_to_dir, ignore_files=ignore_files | ||
| ) | ||
| console = Console(force_terminal=nf_core.utils.rich_force_colors()) | ||
| if current_version is not None and new_version is not None: | ||
|
|
@@ -280,32 +366,44 @@ def print_diff( | |
| for file, (diff_status, diff) in diffs.items(): | ||
| if diff_status == ComponentsDiffer.DiffEnum.UNCHANGED: | ||
| # The files are identical | ||
| log.info(f"'{Path(dsp_from_dir, file)}' is unchanged") | ||
| log.debug(f"'{Path(dsp_from_dir, file)}' is unchanged") | ||
| file_content = "Unchanged" | ||
| elif diff_status == ComponentsDiffer.DiffEnum.CREATED: | ||
| # The file was created between the commits | ||
| log.info(f"'{Path(dsp_from_dir, file)}' was created") | ||
| log.debug(f"'{Path(dsp_from_dir, file)}' was created") | ||
| file_content = "[green]Created[/]" | ||
| elif diff_status == ComponentsDiffer.DiffEnum.REMOVED: | ||
| # The file was removed between the commits | ||
| log.info(f"'{Path(dsp_from_dir, file)}' was removed") | ||
| log.debug(f"'{Path(dsp_from_dir, file)}' was removed") | ||
| file_content = "[red]Removed[/]" | ||
| elif diff_status == ComponentsDiffer.DiffEnum.TO_DECODE_FAIL: | ||
| # We failed to decode the file | ||
| log.debug(f"'{Path(dsp_to_dir, file)}' failed to be decoded") | ||
| file_content = "[bold red]Failed to decode new file[/]" | ||
| elif diff_status == ComponentsDiffer.DiffEnum.FROM_DECODE_FAIL: | ||
| # We failed to decode the file | ||
| log.debug(f"'{Path(dsp_from_dir, file)}' failed to be decoded") | ||
| file_content = "[bold red]Failed to decode old file[/]" | ||
| elif limit_output and not file.suffix == ".nf": | ||
| # Skip printing the diff for files other than main.nf | ||
| log.info(f"Changes in '{Path(component, file)}' but not shown") | ||
| log.debug(f"Changes in '{Path(component, file)}' but not shown") | ||
| file_content = Panel(f"Changes in '{Path(component, file)}' but not shown") | ||
| else: | ||
| # The file has changed | ||
| log.info(f"Changes in '{Path(component, file)}':") | ||
| log.debug(f"Changes in '{Path(component, file)}':") | ||
| # Pretty print the diff using the pygments diff lexer | ||
| syntax = Syntax("".join(diff), "diff", theme="ansi_dark", line_numbers=True) | ||
| panel_group.append(Panel(syntax, title=str(file), title_align="left", padding=0)) | ||
| console.print( | ||
| Panel( | ||
| Group(*panel_group), | ||
| title=f"[white]{str(component)}[/white]", | ||
| title_align="left", | ||
| padding=0, | ||
| border_style="blue", | ||
| box=box.HEAVY, | ||
| ) | ||
| file_content = Syntax("".join(diff), "diff", theme="ansi_dark", line_numbers=True) | ||
| panel_group.append(Panel(file_content, title=str(file), title_align="left", padding=0)) | ||
| console.print( | ||
| Panel( | ||
| Group(*panel_group), | ||
| title=f"[white]{str(component)}[/white]", | ||
| title_align="left", | ||
| padding=0, | ||
| border_style="blue", | ||
| box=box.HEAVY, | ||
| ) | ||
| ) | ||
|
|
||
| @staticmethod | ||
| def per_file_patch(patch_fn: Union[str, Path]) -> dict[str, list[str]]: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
make it a warning or remove