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.

Sign up for GitHub

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

Jump to bottom

Ensure color codes are valid at runtime if running in 16-color mode. #1167

Merged
merged 3 commits into from
Mar 21, 2022
Merged

Ensure color codes are valid at runtime if running in 16-color mode. #1167

merged 3 commits into from
Mar 21, 2022

Conversation

srdeotarse
Copy link
Collaborator

@srdeotarse srdeotarse commented Mar 15, 2022 •
edited
Loading

What does this PR do?
In /zulip-terminal/zulipterminal/config/themes.py code is added in generate_theme function to check if valid color codes are used in color files for specific themes. For eg. (colors_gruvbox.py for gruvbox_dark.py theme)

Fixes #1158

Tested?

  • Manually
  • Existing tests (adapted, if necessary)
  • New tests added (for any new behavior)
  • Passed linting & tests (each commit)

Commit flow

Notes & Questions

Interactions

Visual changes

@zulipbot zulipbot added the size: M [Automatic label added by zulipbot] label Mar 15, 2022
@srdeotarse srdeotarse force-pushed the issue-1158-ensure-color-code-runtime-16-color-mod branch 2 times, most recently from 22d2b8e to c47aee4 Compare March 17, 2022 04:23
@zulipbot zulipbot added size: L [Automatic label added by zulipbot] and removed size: M [Automatic label added by zulipbot] labels Mar 17, 2022
@srdeotarse srdeotarse force-pushed the issue-1158-ensure-color-code-runtime-16-color-mod branch from c47aee4 to fd18000 Compare March 17, 2022 04:35
neiljp
neiljp reviewed Mar 18, 2022
View reviewed changes
Copy link
Collaborator

@neiljp neiljp left a comment

Choose a reason for hiding this comment

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

@srdeotarse Thanks for looking into this 👍

I've left some feedback inline. We'll likely squash the test commit with the other one when merging.

Please do check the commit style guidelines in the README :)
You may also find installing gitlint useful, if you haven't already.

tests/config/test_themes.py
Comment on lines +224 to +208
(16, "zt_dark"),
(16, "gruvbox_dark"),
(16, "gruvbox_light"),
(16, "zt_light"),
(16, "zt_blue"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is useful to use an existing theme as a basis for a test, but I'm not sure we benefit from testing against all the existing themes.

Instead, I'd suggest exploring different Color sets, eg. ensuring that a range of names work, as well as what output one gets if multiple colors in Color are invalid.

@neiljp neiljp added area: colors/styles/themes PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback labels Mar 18, 2022
@zulipbot zulipbot added size: XL [Automatic label added by zulipbot] and removed size: L [Automatic label added by zulipbot] labels Mar 18, 2022
@srdeotarse srdeotarse force-pushed the issue-1158-ensure-color-code-runtime-16-color-mod branch from d9d4925 to 4b5b990 Compare March 18, 2022 17:50
neiljp
neiljp reviewed Mar 18, 2022
View reviewed changes
zulipterminal/config/themes.py Outdated
"""
theme_colors = THEMES[theme_name].Color
failure_text = []
new_line = "\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can join the errors with this instead.

neiljp
neiljp reviewed Mar 18, 2022
View reviewed changes
zulipterminal/config/themes.py Outdated
color_code = c.value.split()[0]
if color_code not in valid_16_color_codes:
invalid_16_color_code = str(c.name)
invalid_16_color_code_name = str(c.value).split()[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just color_code?

@srdeotarse srdeotarse force-pushed the issue-1158-ensure-color-code-runtime-16-color-mod branch 2 times, most recently from 9d47314 to 3718578 Compare March 20, 2022 14:23
@zulipbot zulipbot added size: M [Automatic label added by zulipbot] size: XL [Automatic label added by zulipbot] and removed size: XL [Automatic label added by zulipbot] size: M [Automatic label added by zulipbot] labels Mar 20, 2022
@neiljp neiljp removed the PR awaiting update PR has been reviewed & is awaiting update or response to reviewer feedback label Mar 20, 2022
srdeotarse and others added 3 commits March 20, 2022 14:47
@srdeotarse @neiljp
refactor: test_themes/themes: Migrate valid_16_color_codes into source.
1ef48cb 
These were previously only used in tests; this change allows use at runtime.
@srdeotarse @neiljp
run/themes: When running with 16 colors, ensure color codes are valid.
ca5bfbe 
Adds validate_colors function to check invalid 16code color names.

Tests added.

Fixes zulip#1158.
@neiljp
themes: Simplify error output for bad 16 color codes in themes.
5ab15bd
@neiljp neiljp force-pushed the issue-1158-ensure-color-code-runtime-16-color-mod branch from 138a456 to 5ab15bd Compare March 20, 2022 22:58
@neiljp neiljp added the PR ready to be merged PR has been reviewed & is ready to be merged label Mar 20, 2022
@neiljp neiljp added this to the Next Release milestone Mar 20, 2022
@neiljp
Copy link
Collaborator

neiljp commented Mar 21, 2022

@srdeotarse Thanks for the update; as per discussion in the stream I slightly adjusted some text and added a commit to tidy slightly. Merging now 🎉

@neiljp neiljp merged commit 90d4460 into zulip:main Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neiljp neiljp neiljp left review comments

Assignees
No one assigned
Labels
area: colors/styles/themes PR ready to be merged PR has been reviewed & is ready to be merged size: XL [Automatic label added by zulipbot]
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

Ensure color codes are valid at runtime if running in 16-color mode
3 participants
@srdeotarse @neiljp @zulipbot