Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update builder specification for v0.11.0 #6
Update builder specification for v0.11.0 #6
Changes from 14 commits
86c933d
c4b5397
5adbd9f
a6fea52
4887ee3
5dfd46b
ebbb873
dcf7f47
88c6f14
701454b
9c73029
b9118e5
413ad81
4dac926
ea5f309
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it must have described the computation algorhythm for fallback values in older themes - otherwise it's again doesn't meet the promise of backwar-compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh? I'm not sure I follow. I think we need to define what "backwards compatible" means... can you give examples of what you think we'd be "breaking" with these changes and how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
means:
1.1) so it doesnt have
scheme-kind
field defined2.1) so it have
scheme-kind
field usedscheme-kind
field in such case3.1) so spec must define the logic (algorithm) for such computation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i've just noticed it mentions what fallback values would be computed, it just had different wording so i didn't noticed it before, although it still doesn't describe the algorithm in the spec
i think the formula should be:
"light" if (base00_r+base00_g+base00_b) > (base07_r+base07_g+base07_b) else "dark"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I assume the spec would specify how this happens mathematically - no argument... there are existing luminescence formulas we should probably use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory - yes, but on practice that would en-complicate the spec and so builder implementation notably, whilst r, g, b color computation is already defined in builder spec
and for such comparison the precision of rgb-based color-math is enough
it wouldn't be enough in case of implementing some color transformations a-la base9 or color-mixing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're talking about light or dark we should use a known luminescence formula - not reinvent the wheel. I do not understand why you think using a common, known luminescence calculation would complicate the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
TEMPLATES-DIRECTORY
thetemplates
directory within a template repo or the template repo itself? i.e. https://github.com/base16-project/base16-vim or https://github.com/base16-project/base16-vim/tree/main/templates?Edit: from reading more in the "Output and behaviour" below, it seems this is the template dir (or whatever directory contains the
.mustache
and.config.yaml
files and theconfig.yaml
used a relative path to point to the output dir.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! That's right. The idea is to make it as easy as possible for CI usage (on most repos it is simply
templates
), while allowing for more advanced usage (such as keeping a set of templates somewhere and building them).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to allow for passing a directory of schemes as well to allow building all schemes at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally/alternatively, we should specify that schemes should be loaded (by default) from the base16-schemes repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yet another comment here - one thing I like about the base16-builder-go is the ability to embed a default set of schemes, but allow overriding it if the user wants to clone the schemes repo. Is that something we want to support?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think traversing the directory (should it be recursive or not? is there a max depth? etc) is better handled by the calling shell. That's why
SCHEME-FILE
accepts multiple files. This allows, for example:base16 template $(find . -name *.yaml)
This also allows easier implementation: all files that should be opened are known by the program without listing files:
TEMPLATES-DIRECTORY/config.yaml
SCHEME-FILE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I'd rather not couple the builders with the repo. This would also require all of them to have git as dependencies.
I think that explicit and agnostic is better than implicit and coupled (at least when we're talking about plumbing), so I'd argue that asking the scripter to
git clone
first would be preferrable. Would that make sense?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually just updated this - I recently added a command line flag allowing the user to run in "online mode" which pulls the schemes directly from github (using the tarball endpoint - it was surprisingly easy) and I default to that in the provided Github action, but falls back to the "embedded" schemes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a possible alternative, what would we think of having a --schemes-dir argument (or something similar) and allowing multiple arguments for which templates to process? Or passing the first argument as the schemes dir and everything after that as templates.
Because we've moved to a more consistent base16-schemes repo, we should be taking advantage of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My vote is either a single
templates
directory or calling the CLI multiple times per template (to build all schemes) since again building tons of templates I see as the edge case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really dislike making plumbing tools interact with git or somehow maintain state, so I'd rather be Unixy and make the reference builders just implement the template spec. That is, just build a template + a number of schemes into a number of themes/configs/outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also on the unixyness part, I think iterating over files on a directory is also not very good when we're talking about plumbing tools. There are much better tools available for that (regular shell globbing, find, etc) that can easily be used to get whichever schemes the caller might prefer.
I don't have a strong opinion about building just 1 or multiple templates, as long as we can do something like:
git clone https://github.com/base16-project/base16-schemes git clone https://github.com/base16-project/base16-shell git clone https://github.com/base16-project/base16-vim base16 --templates base16-{shell,vim}/templates --schemes base16-schemes/*.yaml
In this example the builder would (pseudocode):
So, no directory listing, no download stuff, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly off-topic (and another change to add to the list), but what would you think of changing the filename output to an embedded mustache template, defaulting to reading those variables from the config.yaml and the
<output-dir>/base16-<slug><extension>
which currently works?Essentially, replacing all those separate variables with a mustache template and providing a method of backwards compatibility (for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understood. Where would these variables be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, extension and output-dir live in the template repo's templates/config.yaml. We could add an additional variable called filename or output-file, defaulting to a mustache template: {{ output-dir }}/base16-{{ scheme-slug }}{{ extension }}, and allowing users to change the full filename.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about having another variable here for dark/light theme? I saw that vscode requires this in their templates and the way to deal with this at the moment would be to have a script that checks for "-light." for each colorscheme name and then add the value to the colorschemes.
Maybe this is something that should stay on the template repos side, I just thought I'd bring it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a great idea!
If we're asking people to refactor their builders, I don't think it's too much to ask of them. It's actually something I expose with nix-colors (which is basically a nix base16 builder).
We could perhaps specify the strategy used to determine the scheme kind as well, perhaps something simple like: add the three color components together and anything above 382 is dark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, because all the schemes are in 1 repo, we could add
dark: true
to relevant schemes and default todark: false
(or the reverse).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I wouldn't include this line. Maybe it would make sense to move the changelog from the README into this file?
Additionally, maybe it's worthwhile putting our unwritten rule into writing that "spec upgrades should be backwards compatible for templates".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the
0.11
line but keep the0.8
and0.9
one? Or remove both?