feat: add ability to show unified diffs for changed files #562

dstanek · 2022-02-08T06:10:20Z

Signed-off-by: David Stanek [email protected]

The now() template builtin is being used in the test and is an alias for datetime.utcnow(). The now however, just uses datetime.now(). This means that my datetime.now() will always make this test fail. Signed-off-by: David Stanek <[email protected]>

Signed-off-by: David Stanek <[email protected]>

dstanek · 2022-02-08T06:10:48Z

I'm still working on this, but I wanted to get the idea out there.

yajo

the tricky part are updates, they apply some diff without asking.

copier/copier/main.py

Line 705 in e4eccc2

(apply_cmd << diff)(retcode=None)

Gotta do something about that, but anyways that's already a problem, so this won't make it worse.

some code comments too.

very nice addition, thanks!

yajo · 2022-02-08T06:38:03Z

copier/tools.py

+        elif line.startswith("-"):
+            color_print(colorama.Fore.RED, f"{indent}{line}")
+        elif line.startswith("+"):
+            color_print(colorama.Fore.GREEN, f"{indent}{line}")


https://python-prompt-toolkit.readthedocs.io/en/master/pages/printing_text.html#pygments-token-text-tuples with a diff lexer should save a lot of code around here.

So I was going down this path first, but it seemed very difficult to get the result I wanted. I went ahead and finished that work a put together a gist showing the differences. orig.diff is the code from this PR and new.diff uses pygments and prompt_toolkit.

One reason the new code is much larger is that I wanted to have the two-line file header yellow like it is with git. This meant that I had to extend the lexer. In order to add the extra indentation, I had to also extend the PygmentsTokens class to inject whitespace. There may be better ways to accomplish these tasks, but I'm never had to dip into the bowels of these libraries before.

I see.

Well, I'll have to do local testing for both implementations, because code looks complex in both cases, so if we're getting a complexity, I'll get the one that looks/works better 😄

I'll update this implementation here since that's what I'm actively using and I think I can make it cleaner and more concise. If you like the other way better I can switch to that later.

yajo · 2022-02-08T06:41:37Z

copier/tools.py

+
+
+def color_print(color: int, msg: str):
+    print(f"{color}{msg}{colorama.Fore.RESET}")


You have https://python-prompt-toolkit.readthedocs.io/en/master/pages/printing_text.html#html and https://plumbum.readthedocs.io/en/latest/colors.html for printing colorized output. You probably won't need this function.

yajo · 2022-02-08T06:44:58Z

copier/main.py

@@ -144,6 +151,7 @@ class Worker:
    overwrite: bool = False
    pretend: bool = False
    quiet: bool = False
+    diff: bool = False


This is a nice addition. I don't think it's needed to add a new flag for it.

Just default to always diff when we can?

Yes, I can't imagine a use case where the no diff is more useful if we're on interactive mode.

codecov-commenter · 2022-02-08T07:00:35Z

Codecov Report

Merging #562 (57897f6) into master (e4eccc2) will decrease coverage by 0.97%.
The diff coverage is 31.70%.

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
- Coverage   96.29%   95.32%   -0.98%     
==========================================
  Files          41       41              
  Lines        2701     2737      +36     
==========================================
+ Hits         2601     2609       +8     
- Misses        100      128      +28

Flag	Coverage Δ
unittests	`95.32% <31.70%> (-0.98%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
copier/tools.py	`73.45% <20.00%> (-19.41%)`	⬇️
copier/main.py	`92.74% <55.55%> (-1.15%)`	⬇️
copier/cli.py	`96.51% <100.00%> (+0.04%)`	⬆️
tests/test_templated_prompt.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4eccc2...57897f6. Read the comment docs.

pawamoy · 2022-02-09T16:21:37Z

copier/tools.py

+    try:
+        original_lines = get_lines(original_content)
+    except Exception:
+        color_print(
+            colorama.Fore.YELLOW, "diff unavailable: unable to determine encoding"
+        )
+        return
+    try:
+        new_lines = get_lines(new_content)
+    except Exception:
+        color_print(
+            colorama.Fore.YELLOW, "diff unavailable: unable to determine encoding"
+        )
+        return


I know some flake8 plugins would say: "only one line in a try block", but this could probably be factorized as:

Suggested change

try:

original_lines = get_lines(original_content)

except Exception:

color_print(

colorama.Fore.YELLOW, "diff unavailable: unable to determine encoding"

)

return

try:

new_lines = get_lines(new_content)

except Exception:

color_print(

colorama.Fore.YELLOW, "diff unavailable: unable to determine encoding"

)

return

try:

original_lines = get_lines(original_content)

new_lines = get_lines(new_content)

except Exception:

color_print(

colorama.Fore.YELLOW, "diff unavailable: unable to determine encoding"

)

return

Also, is there a reason you catch Exception rather than the more specific UnicodeDecodeError?

I could combine the try blocks.

The reason I'm currently catching Exception is mostly because this isn't ready to merge. My first version used chardet and there were some exceptions to catch, then I decided to do a "detect on the cheap" algorithm to avoid an extra dependency and finally settled on hard coding utf-8 once I realized that Jinja is already doing that. If the final version is just the single decode then I could switch to catch that one exception.

Since the previous implementations had a few exceptions to catch and there is no particular handing for any of them I just went with Exception.

I see, thanks!

yajo · 2022-03-28T20:07:52Z

Hi folks! This was stiill under my radar, although quite deferred because it needs some testing and deep analyzing.

However, recently we got #627 which proposes to remove --overwrite and instead let the user solve the diff after updating. IMHO that seems a simpler approach, both in code and in behavior.

I'd love to hear your opinions.

dstanek · 2022-04-08T12:56:55Z

Hi folks! This was stiill under my radar, although quite deferred because it needs some testing and deep analyzing.

However, recently we got #627 which proposes to remove --overwrite and instead let the user solve the diff after updating. IMHO that seems a simpler approach, both in code and in behavior.

I'd love to hear your opinions.

I'll take a look at that in more detail, but it doesn't seem related to this PR. In this PR I wanted the ability do what Ansible calls "check mode." The ability to run a command and see what would change if it were to be applied.

Just a little background on my use case. I have a set of a few dozen microservices (likely to keep growing) that are based on the same configuration files and general structure. I want to convert it to use copier as the base for management. New services would use copier to start off. With this feature, we could make changes to our base configuration repo (say a logging format change) and have an easy way for a service owner to see what is different without creating new files. We could also run this over all services to see which is out of sync (run a weekly report from CICD like dependabot).

dstanek added 2 commits February 7, 2022 23:58

feat: add ability to show unified diffs for changed files

57897f6

Signed-off-by: David Stanek <[email protected]>

yajo reviewed Feb 8, 2022

View reviewed changes

yajo mentioned this pull request Feb 9, 2022

test: fix date comparison not working in my timezone #561

Merged

pawamoy reviewed Feb 9, 2022

View reviewed changes

yajo mentioned this pull request Mar 28, 2022

Improve conflict resolution #627

Closed

dstanek added 2 commits April 8, 2022 12:13

Merge remote-tracking branch 'upstream/master' into diff-changes

bef089c

fix: Remove customer color_print function

7e7e1a7

dstanek force-pushed the diff-changes branch from 24f2520 to 7e7e1a7 Compare April 8, 2022 23:08

pawamoy mentioned this pull request Nov 23, 2022

Suggested improvements to the update process #877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add ability to show unified diffs for changed files #562

feat: add ability to show unified diffs for changed files #562

dstanek commented Feb 8, 2022

dstanek commented Feb 8, 2022

yajo left a comment •

edited

Loading

yajo Feb 8, 2022

dstanek Feb 9, 2022

yajo Feb 10, 2022

dstanek Feb 10, 2022

yajo Feb 8, 2022

yajo Feb 8, 2022

dstanek Feb 9, 2022

yajo Feb 10, 2022

codecov-commenter commented Feb 8, 2022 •

edited

Loading

pawamoy Feb 9, 2022

dstanek Feb 9, 2022 •

edited

Loading

pawamoy Feb 9, 2022

yajo commented Mar 28, 2022

dstanek commented Apr 8, 2022



		def color_print(color: int, msg: str):
		print(f"{color}{msg}{colorama.Fore.RESET}")

feat: add ability to show unified diffs for changed files #562

Are you sure you want to change the base?

feat: add ability to show unified diffs for changed files #562

Conversation

dstanek commented Feb 8, 2022

dstanek commented Feb 8, 2022

yajo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 8, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dstanek Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yajo commented Mar 28, 2022

dstanek commented Apr 8, 2022

yajo left a comment •

edited

Loading

codecov-commenter commented Feb 8, 2022 •

edited

Loading

dstanek Feb 9, 2022 •

edited

Loading