Skip to content
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

✨ Syntax highlighted diff output ✨ #4

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

Shivansh-007
Copy link

A bit hacky but it works, I haven't completely tested it with diff-shades but have tested the _diff.py lib separately.

Btw, I am logged out of discord and can't log in back (my phone broke and I can't find the recovery codes 😔), so keep the conversation here.

And happy new year! (a bit late)

@ichard26
Copy link
Owner

ichard26 commented Jan 4, 2022

Thanks for working on this. I'm not a big fan of how much complexity this introduces for just the diff functionality especially as I'm a bit worried about the maintainability of it. I actually just had another idea how do this inspired from this article. It's pretty likely it won't work as I'd want it to but if it does the benefits are 1) way less hacky code, and 2) correct behaviour with multiline strings and similar constructs. Performance will suffer but performance doesn't matter too much if we're displaying a colored diff to the user anyway. I'll look into this later this week. actually I don't think this will work as it doesn't account for inline colors in the highlighted lines >.<

Concretely, this branch is a bit out of date (which is important because I just refactored the whole test suite), it needs some typing fixes, and quite a bit of glue code with the commands to handle light themed terminals. I can take over and see if I can resolve these issues myself. I know I've described diff-shades as a hacky bit of software but I'm trying to slowly undo that (eg. see the brand new test suite on main) 🙂

@Shivansh-007
Copy link
Author

Sure, that would be great. Let me know if you come across any difficulty 👍

@Shivansh-007
Copy link
Author

Light

Screenshot from 2022-01-12 06-53-13

Dark

Screenshot from 2022-01-12 06-53-35

@Shivansh-007
Copy link
Author

Would appreciate another look at this, bumping in case you forgot this PR exists :D

@ichard26 ichard26 force-pushed the main branch 3 times, most recently from 9f8c993 to 6ec675b Compare March 23, 2022 22:32
@Shivansh-007
Copy link
Author

Hey @ichard26, hope you are doing well. Do you have any plans on proceeding with this addition?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants