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

Start on a Legacy submodule for backwards compat #3

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

tecosaur
Copy link
Collaborator

As we change more code to use the StyledStrings approach, it would be nice to try to maintain compatibility with old user customisations as much as possible.

This is currently an incomplete solution, but as a start we can add the capability to load 256-color values and respect (some of) the JULIA_*_COLOR environment variables.

@tecosaur tecosaur force-pushed the legacy-color-loading branch from af1be78 to ca80aea Compare October 22, 2023 06:41
Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Is this code copied from somewhere or just emulating the old stuff with a new implementation? If it's copied code, then leaving it as identical as possible is good (for comparability); if it's new code emulating old behavior, it makes more sense to comment on style.

src/legacy.jl Outdated Show resolved Hide resolved
@tecosaur
Copy link
Collaborator Author

This is new code to emulate prior behaviour, so I'm happy to take pointers on style.

@tecosaur tecosaur force-pushed the legacy-color-loading branch 2 times, most recently from a682673 to 6423025 Compare October 23, 2023 15:13
@StefanKarpinski
Copy link
Member

Don't care for the ternary operator, huh?

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

A couple of suggested changes, one possible typo; maybe needs more tests so that typos would be caught?

src/legacy.jl Outdated Show resolved Hide resolved
src/legacy.jl Outdated Show resolved Hide resolved
src/legacy.jl Outdated Show resolved Hide resolved
@tecosaur tecosaur force-pushed the legacy-color-loading branch from 6423025 to ba8ca7b Compare October 24, 2023 00:43
@tecosaur
Copy link
Collaborator Author

tecosaur commented Oct 24, 2023

Thanks for giving this another/closer look Stefan. I appreciate you giving some of your time to this.

Just addressed all of your comments, since it seems like the general idea/design here is fine I'll write some tests for this before thinking of merging.

(Aside: I may hold the opinion that the ternary operator is a mistake, and never should have been added 🌶️ 🌶️)

@tecosaur tecosaur force-pushed the legacy-color-loading branch from ba8ca7b to eafe890 Compare October 25, 2023 15:27
@tecosaur
Copy link
Collaborator Author

Just added tests, and fixed two issues that became apparent over the course of doing so.

Since there have been no comments against this, other than code style improvements (which have been acted on), I'll be going ahead and merging this shortly.

As we change more code to use the StyledStrings approach, it would be
nice to try to maintain compatibility with old user customisations as
much as possible.
@tecosaur tecosaur force-pushed the legacy-color-loading branch from eafe890 to bf2ce26 Compare October 25, 2023 15:32
@tecosaur tecosaur merged commit 1d9acc7 into main Oct 25, 2023
@tecosaur tecosaur deleted the legacy-color-loading branch October 25, 2023 15:57
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