GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-133722: Add Difflib theme to `_colorize` and 'color' option to `difflib.unified_diff` by dougthor42 · Pull Request #133725 · python/cpython Conversation
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks for the PR! With the basic theming support in #133347 merged, perhaps we could define a separate difflib theme and use that here? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Thanks for the PR! I had a play with something similar but didn't get very far before the 3.14 freeze.
Yes, let's add theming support. See this docstring for instructions. For a couple of standalone examples of adding theme support to a module, see 230d658 and 5c5c3e1 from that PR. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ah, TIL! Thanks, I'll take a look at those docs and update accordingly. It'll be a few days a least before I can get to it, so for now I'll revert this back to draft. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Absolutely no rush for this, we have 12 months until the 3.15 feature freeze. Don't hesitate if you have questions :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Wow that was really easy! The examples helped a lot too, thanks! PTAL at your leisure.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
_colorize and 'color' option to difflib.unified_diff
| equal: str = ANSIColors.RESET # context lines | ||
| insert: str = ANSIColors.GREEN | ||
| delete: str = ANSIColors.RED |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I opted to use the difflib.context_diff internal terminology here. Should I use the more git-like terms context, added, and removed instead?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe the Git-like terms if they're going to be more familiar with people and if the difflib ones are all internal.
Where does Git mention them?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall they are defined in the GNU unified diff detailed description.
The git diff man page mentions them in a variety of places (emphasis mine):
-U
--unified=Generate diffs with lines of context instead of the usual three. Implies --patch.
Generating patch text with -p
- Hunk headers mention the name of the function to which the hunk applies.
plain
Any line that is added in one location and was removed in another location will be colored with
color.diff.newMoved.
Though sometimes it uses "deleted" instead of "removed":
--numstat
Similar to --stat, but shows number of added and deleted lines
I've gone ahead and switched to the GNU unified diff terms prior to the requested sorting. Also, to keep consistent with the other dataclass attributes (which are not currently sorted), the reset value was left at the end.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hm, I would expect to see this in the difflib CLI rather than a library function. But, as it turns out, difflib doesn't have a working CLI? That's different than what's documented.
I personally think it's worth adding one if we're going through the effort to add colorization.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Let's also add a What's New entry, perhaps something similar to https://docs.python.org/3.14/whatsnew/3.14.html#whatsnew314-color-argparse
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| equal: str = ANSIColors.RESET # context lines | ||
| insert: str = ANSIColors.GREEN | ||
| delete: str = ANSIColors.RED |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe the Git-like terms if they're going to be more familiar with people and if the difflib ones are all internal.
Where does Git mention them?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| syntax: Syntax = field(default_factory=Syntax) | ||
| traceback: Traceback = field(default_factory=Traceback) | ||
| unittest: Unittest = field(default_factory=Unittest) | ||
| difflib: Difflib = field(default_factory=Difflib) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
And here:
| syntax: Syntax = field(default_factory=Syntax) | |
| traceback: Traceback = field(default_factory=Traceback) | |
| unittest: Unittest = field(default_factory=Unittest) | |
| difflib: Difflib = field(default_factory=Difflib) | |
| difflib: Difflib = field(default_factory=Difflib) | |
| syntax: Syntax = field(default_factory=Syntax) | |
| traceback: Traceback = field(default_factory=Traceback) | |
| unittest: Unittest = field(default_factory=Unittest) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm very slightly concerned about sorting when the dataclasses aren't kw_only.
Should I update the dataclass decorator to include kw_only=True and then sort? Or is that out of scope of this PR?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Let's ask @ambv about this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
👍 Done
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| Added a ``color`` option to :func:`difflib.unified_diff` that injects ANSI color | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| Added a ``color`` option to :func:`difflib.unified_diff` that injects ANSI color | |
| Added a *color* option to :func:`difflib.unified_diff` that injects ANSI color |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Still todo :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Oops!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
What's New entry
Added, assuming a 3.15 release.
I would expect to see this in the difflib CLI rather than a library function.
My underlying use case and request is specific to the library function. That said, I agree that any CLI should also include coloring.
difflib doesn't have a working CLI? That's different than what's documented.
Hmm... I'm not able to find the difflib CLI docs? The closest thing I found was https://docs.python.org/3/library/difflib.html#difflib-interface but that just looks like an example of how to make a CLI for difflib. Was that what you were thinking of?
If I actually read the comment I would have seen that the docs were indeed linked... And yes, the linked item is just an example of how one might make a CLI for difflib.
I personally think it's worth adding one if we're going through the effort to add colorization.
I'd be happy to modify any existing CLI code to include colors. I can't say I'd have the time to add a CLI though.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| equal: str = ANSIColors.RESET # context lines | ||
| insert: str = ANSIColors.GREEN | ||
| delete: str = ANSIColors.RED |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Overall they are defined in the GNU unified diff detailed description.
The git diff man page mentions them in a variety of places (emphasis mine):
-U
--unified=Generate diffs with lines of context instead of the usual three. Implies --patch.
Generating patch text with -p
- Hunk headers mention the name of the function to which the hunk applies.
plain
Any line that is added in one location and was removed in another location will be colored with
color.diff.newMoved.
Though sometimes it uses "deleted" instead of "removed":
--numstat
Similar to --stat, but shows number of added and deleted lines
I've gone ahead and switched to the GNU unified diff terms prior to the requested sorting. Also, to keep consistent with the other dataclass attributes (which are not currently sorted), the reset value was left at the end.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| syntax: Syntax = field(default_factory=Syntax) | ||
| traceback: Traceback = field(default_factory=Traceback) | ||
| unittest: Unittest = field(default_factory=Unittest) | ||
| difflib: Difflib = field(default_factory=Difflib) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I'm very slightly concerned about sorting when the dataclasses aren't kw_only.
Should I update the dataclass decorator to include kw_only=True and then sort? Or is that out of scope of this PR?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| Added a ``color`` option to :func:`difflib.unified_diff` that injects ANSI color | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
@ZeroIntensity Doh! I just re-read your comment and realized you linked to it haha. Anyway yeah that's just example code. To my knowledge there is no official CLI for |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| syntax: Syntax = field(default_factory=Syntax) | ||
| traceback: Traceback = field(default_factory=Traceback) | ||
| unittest: Unittest = field(default_factory=Unittest) | ||
| difflib: Difflib = field(default_factory=Difflib) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Let's ask @ambv about this.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| @@ -0,0 +1,2 @@ | |||
| Added a ``color`` option to :func:`difflib.unified_diff` that injects ANSI color | |||
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Still todo :)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This PR needs to be updated to resolve the conflicts → #134581 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
ACK @Wulian233. I was planning on waiting to resolve conflicts until after @ambv comments on #133725 (comment). |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@dougthor42 I responded on that comment. It's a good suggestion to make the dataclasses kwonly. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| Set *color* to ``True`` to enable output in color, similar to | ||
| :program:`git diff --color`. Even if enabled, it can be | ||
| :ref:`controlled using environment variables <using-on-controlling-color>`. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
The CPython docstrings aren't rendered into docs, so let's use plain formatting like elsewhere. Perhaps:
| Set *color* to ``True`` to enable output in color, similar to | |
| :program:`git diff --color`. Even if enabled, it can be | |
| :ref:`controlled using environment variables <using-on-controlling-color>`. | |
| Set 'color' to True to enable output in color, similar to | |
| 'git diff --color'. Even if enabled, it can be | |
| controlled using environment variables such as 'NO_COLOR'. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Done. Also fixed the 3 spaces thing 🤦
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thank you! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…to `difflib.unified_diff` (python#133725)
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
Add acolor: bool arg to difflib.unified_diff
There was an error while loading. Please reload this page.