Improve Theme Structure #1051
Improve Theme Structure #1051
Conversation
|
I've run into a circular import issue in |
Rohitth007
changed the title
There was a problem hiding this comment.
@Rohitth007 Extracting the colors - however we end up storing them - makes this a lot more compact and readable 👍
I left some ideas on adjusting the structure further which could simplify/clarify the processing.
It would be useful to see some better commit structure here - this looks just like the way you're layering on the changes? I just reviewed the net result ("files"); intermediate states of development can be useful, but generally reviewers are more interested in how you're layering the changes upon each other intentionally rather than over time (at least in a branch/PR).
While the automation using the script is good to see, if we're going to migrate to this then I'll reiterate that I would like to see a programmatic test that compares the existing and new results for themes at each color depth - rather than relying upon manual comparisons. We might only have that test present after the first few commit(s) and then remove it with the old code, but it would ensure that the new code is good while we have both present.
Layout-wise, I'm not sure yet where we'll want the end result, but for now you could consider putting the generate-theme code in run.py to avoid the import issue?
neiljp
added
the
PR awaiting update
|
Most of the work is done, I just have to add few more tests. |
Rohitth007
force-pushed
the
theme-reorg
branch
2 times, most recently
from
d702c8f
to
299f249
Compare
|
All tests are updated, only linting issues are left as mentioned in the above comment. Can we disable this one for the files? |
Rohitth007
added
PR needs review
|
I have added E221 to the ignore list of flake8 |
|
I figured it out!! But I had to ignore 1 error as that is a Mypy issue. |
Rohitth007
added
the
PR ready to be merged
|
There are some comments to be resolved but this is ready to be merged I think. |
neiljp
added
PR awaiting update
There was a problem hiding this comment.
@Rohitth007 This is looking really good and the intermediate tests will be useful (though I've not tested them manually), but this is quite a large refactor - bigger than you originally thought, perhaps? :)
The challenge with a larger PR is that lots of smaller tidying/refactoring can be identified along the way, which is what makes this more tricky to handle and get merged as quickly. This will definitely improve our theme handling, we just want to do it well 👍
Rohitth007
removed
the
PR awaiting update
Rohitth007
added
PR needs review
There was a problem hiding this comment.
@Rohitth007 Just some quick feedback as I wasn't sure if it was ready and I'm off to bed.
I couldn't answer all your questions above after I started a review, but I think I answered them below?
neiljp
added
PR awaiting update
|
@neiljp I've updated this with the suggestions received. I've also had to change |
Rohitth007
added
PR needs review
A helper function `color_properties` was used to make it easy to add color definitions with style properties. It was added into config/color.py to avoid circular imports in `themes.py`. Tests added.
A DefaultColor class is added to color.py which acts as the default color-scheme for ZT. The BOLD property is added to these colors using the helper function color_properties. The format: * The colors have been defined using Enum's as a single string containing 16, 256 and 24 bit codes. * Each style is defined as a tuple of 2 elements, the foreground and the background. The 24 bit colors are not defined for the default colors hence default is used. 2 missing colors in DEF, DARK_MAGENTA and LIGHT_CYAN, which were used in zt_blue were added into Color.
The `required_styles` dict is renamed to REQUIRED_STYLES to maintain similarity with STYLE dict in incoming themefiles. Tests updated.
This commit creates a Theme Contribution Guide to help with understanding the format in Themefiles and creating new themes.
New zt_dark theme structure was created using a script and some manual editing. `zt_dark.py` can be found in the new themes folder.
New zt_light theme structure was created using a script and some manual editing. `zt_light.py` can be found in the themes folder.
New zt_blue theme structure was created using a script and some manual editing. `zt_blue.py` can be found in the themes folder.
New gruvbox theme structure was created using a script and some manual editing. `gruvbox.py` can be found in the themes folder. The names used for the color scheme is from the official gruvbox theme.
This commit tests completeness on new themefiles in test_themes. Checks: * STYLE and REQUIRED_STYLES use the same styles * All 3 color codes are in the correct format, using regex * color used in STYLE is defined in the Color class. Run: pytest -k test_new_builtin_theme
This commit creates the function `generate_theme` in themes.py which parses a theme file to generate a urwid compatible theme depending on the color-depth used. The theme files are accessed using NEW_THEMES which is a placeholder for the THEMES dict until migration happens. A new type hint for urwid compatible styles is created - StyleSpec. Slicing lists doesn't work with very specific type hints like StyleSpec hence Any is used, though this function will shortly be removed. Tests added.
A temporary test has been used to make sure all the color codes for all the color depths are correct and no errors have been made during the transition. Run: `pytest -k test_migrated_themes`
Old theme format used in themes.py is deleted from this commit. * Migrated to new themes in `run.py` * Mono theme generation is no longer required hence it is removed from both themes.py and run.py * NEW_THEMES is now the new THEMES dict. * `complete_and_incomplete_themes` is edited to make it use the new THEMES instead of the old one. A new `builtin_theme_completeness` test is added. Tests amended.
|
@Rohitth007 I moved that chunk of code between commits we discussed as well as a test that you removed in an earlier commit rather than in the last, and some very minor documentation updates to take into account where color.py is now. After adjusting some commit text I just pushed here and will merge shortly - this will be great to have in, thanks for working on it! 🎉 There are quite a few follow-ups we could explore, though most are not super-important right now and we might file as issues (except the last few):
|
This PR transitions to a different structure for specifying themes. A new themes folder is created that contains all the themes and instructions on how to create a new one.
The commits are as follows: