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

Update builder specification for v0.11.0 #6

Conversation

Misterio77
Copy link
Member

@Misterio77 Misterio77 commented Jun 22, 2022

Hey!

Based on my comment on #3, I've decided to go ahead and make a draft of what I had in mind, in the form of a reworked builder specification.

I think I'm done writing (there may be some typos around or wording fixes), so I'm ready for review and comments!

Please let me know what you think :)

Rationale

  • Builders have interfaces (arguments, etc) that are completely different between one another, yet mostly do the same thing. This makes any CI using them coupled to the specific builder, not the formal specification.
  • Every single software using base16 somehow has their own base16 implementation, we should try and provide libraries for this. Builders could act as both a CLI and software library for their respective languages, with minimal additional work.
  • The current specification is very loose and uses a lot of "shoulds", so it's completely left to what the author wants to do, making standardization more difficult.
  • There is a blurred line between a software that "builds" and "manages" base16 templates/schemes.
    • The current specification says that builders must fetch stuff. This is awkward to use when scripting (you have to manipulate data dirs repeately)
    • For the software more geared towards users, the "update" workflow is also very awkward and makes it really hard for users to manage their overrides. When we had a template index, it was really easy to delete custom templates.
  • I propose we make the definition 100% clear. Managers are for end users, contain opinionated workflows, and can do pretty much whatever the author thinks that makes sense to their usage. While builders are highly scoped, consistent, and very, very easy to script with.

Expected results

  • Builders that are dead-simple to use with CI
  • The manager definition is made clear
  • Software that uses base16 will be easier to implement, be more correct and more interoperable (we can, for example, build a library to generate actual base16 schemes from images).
  • Make the builder document as unambiguous as possible, while being as easy as possible to read
  • This will not introduce any incompatibility between schemes, templates, and existing builders. The only thing changing is the recommended way of writing an official builder.

Changes

  • I'm proposing we make builders only build templates. It is now required that builders (at least the features exposed via CLI) only offer build functionality, not management or anything else.
  • The idea is that, as they are a plumbing tool, they should follow a strict interface and behaviour standard, making them fully interoperable and avoiding protocol extension (improvements should instead be contributed back here). It's not a law binding, of course, but I think being fully compliant should be a requirement for endorsement.
  • There is also an optional interface the builder may implement, in the form of a software library for their respective language/platform. This should help everyone that makes base16-compatible software!
  • As the builders no longer are designed to fetch repositories, the template directory (containing config.yaml and .mustache's) and scheme list are required arguments.
  • When we have a slug conflict, which is very unusual I'd say, I've specified that the program must instead exit with an error. This should be easier to script with, as well as very easy to implement, as all scheme files (thus their slugs) are specified as an argument.
  • I've made hex and hex-bgr mandatory, to encourage 100% compatibility between builders.
  • I've adopted the RFC 2119 keyword description. The previous specification contained a lot of "shoulds" where it should have been "must".
  • The specification is a lot more strict now. It specifies exactly what arguments the CLI has, and its full behaviour.

Questions

  • Should we adopt @belak's slug-underscored? I believe it makes sense as to avoid builders not having it.
  • Is my idea of erroring out when the file exists okay? Or is there a specific reason for the current behaviour? Warnings are quite hard to script with
  • Does it make sense (added a todo for it) to write an official manpage and test suite for the builders? I like it but would like feedback on that before commiting to it.

Copy link
Member

@JamyGolden JamyGolden left a comment

Choose a reason for hiding this comment

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

Nice work! I've added a few questions throughout the file.

Should we adopt @belak's slug-underscored? I believe it makes sense as to avoid builders not having it.

I think it makes sense.

Is my idea of erroring out when the file exists okay? Or is there a specific reason for the current behaviour? Warnings are quite hard to script with

I think the erroring out with an optional message makes sense. We could even have suggested messages, but the error code is important.

Does it make sense (added a todo for it) to write an official manpage and test suite for the builders? I like it but would like feedback on that before commiting to it.

I think a test suite would be really great. Tests are important for everyone. It would also be nice to have a manpage so users could understand the cli without needing to leave the command line. How are manpages managed? (I've never written one). Would we have it exist in this repo?

builder.md Outdated Show resolved Hide resolved
```

`TEMPLATES-DIRETORY` being a directory containing a `config.yaml`, as well as at
least one `.mustache` template. These are usually named `templates` on all
Copy link
Member

Choose a reason for hiding this comment

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

Is this TEMPLATES-DIRECTORY the templates 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 the config.yaml used a relative path to point to the output dir.

Copy link
Member Author

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).

builder.md Outdated Show resolved Hide resolved
builder.md Outdated

### Library

The compliant base16 builder software MAY also expose a software library other
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by this? Can you give an example?

Copy link
Member Author

@Misterio77 Misterio77 Jun 25, 2022

Choose a reason for hiding this comment

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

Sure.

Suppose you created a python builder. It probably has some kind of class for representing schemes or templates, possibly method or functions for color conversion or the template replacement itself.

It could be useful for other python programs, for example, a GUI that uses base16 to build some sort of theme. The creator could avoid writing code to implement the base16 standard (parsing schemes, messing with the template, etc) if there was some kind of library they could just import the functionality from.

If the base16 is already very general purpose, why not expose your classes/functions/methods as a library for other python software to use?

builder.md Outdated
replaced with dashes and both the extension and output-dir are taken from
`config.yaml`.

The builder MUST check for the (unusual) case where schemes share the same
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't (shouldn't?) we test for this in the schemes repo? Have some tests running and include this case? I suppose we should still check for this here because someone could clone and edit the schemes dir while testing out a colourscheme and give duplicate a slug value.

Copy link
Member Author

@Misterio77 Misterio77 Jun 25, 2022

Choose a reason for hiding this comment

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

Yeah, we shouldn't have any schemes with colliding slugs (actually, all file names on our repo should already be slugified).

And yup, that's exactly it. It could be useful to error out when someone with a modified scheme collection accidentally refers to 2+ schemes sharing same slug, which would be very hard to debug.

My idea is basically erroring out only when the inputs are the same, the outputs should be overwritten silently; as it is a common usecase to run the command to rebuild when the output already exists, and current builders spam a lot of warnings that are only useful to debug that rare collision problem.


- `scheme-name` - obtained from the scheme file
- `scheme-author` - obtained from the scheme file
- `scheme-slug` - obtained from the scheme filename, as described above

Copy link
Member

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.

Copy link
Member Author

@Misterio77 Misterio77 Jun 25, 2022

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?

Copy link
Member

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 to dark: false (or the reverse).

builder.md Outdated Show resolved Hide resolved
Misterio77 and others added 2 commits June 25, 2022 18:29
Co-authored-by: Jamy <jamy@jamygolden.com>
Co-authored-by: Jamy <jamy@jamygolden.com>
builder.md Outdated Show resolved Hide resolved
builder.md Show resolved Hide resolved
builder.md Outdated

In the case where schemes share the same slug, a builder will overwrite files previously generated from the template. Should this happen, a builder should show warning messages listing the overwritten files.
It is REQUIRED that that this functionality is exposed as binary CLI executable
Copy link
Contributor

Choose a reason for hiding this comment

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

Does CLI need to be required for builder? I propose we just have one CLI implementation. Other builders just act as a library for different languages. And the CLI implementation can be thought of as a library for bash. And details of what the CLI looks like is a decision for the sole CLI implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can require the builder to expose its functionality as CLI and/or Library.

I'm not sure if a single CLI implementation would be very good. What implementation should be the canonical one? And it's pretty easy to implement a CLI after you have the library, so perhaps it isn't too much of a requirement.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if a single CLI implementation would be very good.

Why?

Could you explain why we need all builder to have a CLI? If it's a CLI and has the same interface, why does a user want to one verse another?
I'm not against specifying what the cli should do in the spec, but just against requiring all builder to have one.

Having only one CLI implementation was a follow up point. If there is reason to have more than one implementation, it still doesn't mean we should require all builders to have a CLI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should move discussion to #13

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I started that before I saw any of this.

@belak
Copy link
Member

belak commented Jun 26, 2022

Thank-you for taking the time to submit this!

Please don't take the rest of this comment too harshly - I'm trying to get my thoughts out and explain where I'm coming from, while making my point for a number of changes. Overall I'm a fan of formalizing the language and cleaning it up, but I have some concerns that the scope of this document is expanding in some ways that could be fairly limiting to builders and managers.

When I was the sole contributor with commit access to the spec, I wanted the document to be about what a builder had to support in order to take a template repo and build out the schemes. Everything else was out of scope. I'm not completely opposed to limiting what command line arguments a builder can take, but originally that would have been out of scope. If that's what we decide as a group to do, we can move forward with it, but I would really prefer to keep "what a builder needs to support in order to build base16 templates" and "a common base16 builder CLI" separate. These changes start mixing the two together.

I'll go over my thoughts about the goals as mentioned in the description:

Builders that are dead-simple to use with CI

I understand why this would be desirable, but I personally don't know if this should be a goal of builders. I have gone out of my way to make base16-builder-go easily usable in CI (I recently pushed a change to add support for GitHub actions and this is what we're currently using for all the base16-project repos), but I'm not sure this should be the goal of every builder.

As mentioned above, I don't particularly agree with the change to be explicit about only allowing certain command line flags with the builders. I think that limits them, and potentially limits the usefulness of them as well as discourages people from experimenting and doing interesting things.

Originally the builder spec had requirements about build and update subcommands, which limited the changes we could make - it meant every builder needed to be aware of the old git repo hierarchy (base16-schemes-source -> scheme repos as well as base16-templates-source -> template repos) and had to provide a method of updating everything. I removed these when I started making changes to the spec, because I found that I was using my builder primarily to build 1 template at a time, not all of them.

I'm still not a fan of it, for the above reasons, but if it's decided that we want to go with a CLI spec as well, I'd push for using it under the build subcommand as a compromise.

The manager definition is made clear

Is it useful to define manager as "a program that does something with base16 schemes which doesn't necessarily follow a spec"? It seems like an odd thing to specify in the builder spec.

I'd also personally be fine with a manager including an implementation of a builder so they could have a fully integrated solution for end users (clone, build, and install templates). However, for a manager to work like that, it would need to implement the spec, which isn't possible under the current set of changes (because the CLI definition is very limited). This is one of the reasons I would push for defining what's needed to build templates and a CLI spec separately.

Software that uses base16 will be easier to implement, be more correct and more interoperable (we can, for example, build a library to generate actual base16 schemes from images).

Interoperability is a good goal, but also limits what people can do. I wrote base16-builder-go with the goal of making it easy to use, and useful for template maintainers. I'd worry that limiting builders like this could force builders to all focus on a single use case when there are others out there we haven't realized.

I do think it makes sense to encourage maintainers of builders to export builder functionality as a library, for the sake of interoperability, but I'm not sure I'd put that in the spec itself.

Make the builder document as unambiguous as possible, while being as easy as possible to read.
This will not introduce any incompatibility between schemes, templates, and existing builders. The > only thing changing is the recommended way of writing an official builder.

This part, I definitely agree with - much of the spec has been cobbled together and has been long in need of a cleanup.

@belak
Copy link
Member

belak commented Jun 26, 2022

Other questions:

Should we adopt @belak's slug-underscored? I believe it makes sense as to avoid builders not having it.

This seems like a good idea. I don't remember where it was originally requested, but it was added at the request of someone maintaining a template originally.

Is my idea of erroring out when the file exists okay? Or is there a specific reason for the current behaviour? Warnings are quite hard to script with

I think this is no longer important because it's impossible to have multiple schemes with the same slug now, which should mean it's impossible to write multiple files with the same name.

Does it make sense (added a todo for it) to write an official manpage and test suite for the builders? I like it but would like feedback on that before commiting to it.

Yes, test suite for sure. I'm a fan of https://github.com/ircdocs/parser-tests - they provided all the tests in a language-agnostic way which could be imported and used for any of the builders.

@Misterio77
Copy link
Member Author

Thank-you for taking the time to submit this!

You're very welcome! Sorry if I mixed spec and strictness together, I see that this is a quite an issue now.

Please don't take the rest of this comment too harshly

No problem at all! Please keep up the feedback, we can get a real nice spec going!


My general idea is introducing uniform CLI/libraries to make it easier to build base16-related tools that leverage the build functionality:

  • Creating performant base16 bash tools becomes possible, as they can simply call whatever base16 binary the user has installed (as they all would work the same).
  • Ditto for CI tools. Any and every CI tool could build a template using a single command, regardless of what core builder it is using.
    • Making specific builder<--->CI tool compatibility would be just an optional convenience.
  • Most programming languages would have a builder library, meaning they would only need to implement their porcelain
    • It's okay, of course, to also implement the builder functionality, but having other features means they can't be used as a dependency so not considered a "reference builder".

I would really prefer to keep "what a builder needs to support in order to build base16 templates" and "a common base16 builder CLI" separate

I think it makes sense.

If I understand correctly we could define what a reference/core/canonical builder should be, instead? This being more strict (only do that specific thing, minimal dependencies, uniform CLI, expose library, etc)?

I was thinking about maybe making a clear split (different sections?) between "builder functionality specification" and "reference builder specification". Would that make sense?

Maybe we could rename these reference builders to something like "reference/canonical base16 implementations"? So that software that extend them could also be named builders.

I have gone out of my way to make base16-builder-go easily usable in CI (I recently pushed a change to add support for GitHub actions and this is what we're currently using for all the base16-project repos), but I'm not sure this should be the goal of every builder.

This would be high level functionality, aka porcelain! In this case the you could perhaps factor out your code that does the actual template building into a go library with the thinnest interface possible, making it very easy for people to use (and keep up to date with our spec) that functionality to create things that you perhaps wouldn't want in your own tool.

I think that limits them, and potentially limits the usefulness of them as well as discourages people from experimenting and doing interesting things.

In this PR's current state, this is actually encouraged, as building interesting things wouldn't require implementing the same old "build template" functionality.

The idea was to categorize these programs in a broad "base16 tools" section, while keep a core set of stable, light, well-maintained, high quality, unopinionated builders.

This also makes it easy for us to extend the format in the future, as we would just update the core builders and the rest of the ecossystem would get new features effortlessly (other than simply bumping a dependency).

However, for a manager to work like that, it would need to implement the spec, which isn't possible under the current set of changes (because the CLI definition is very limited).

They can implement the spec if they want (or even better, split it into a lib), but having a less stable and uniform interface makes it harder to script with, so they would be porcelain instead (opinionated use cases).

I think this is no longer important because it's impossible to have multiple schemes with the same slug now, which should mean it's impossible to write multiple files with the same name.

ACK! Gonna go ahead and remove that part right away.

Yes, test suite for sure. I'm a fan of https://github.com/ircdocs/parser-tests - they provided all the tests in a language-agnostic way which could be imported and used for any of the builders.

Seems nice! The easier it is to import the tests to each languages' preferred test suits, the better.

@Misterio77
Copy link
Member Author

I was thinking about maybe making a clear split (different sections?) between "builder functionality specification" and "reference builder specification". Would that make sense?

I'm gonna go ahead and try getting this written, actually

@Misterio77
Copy link
Member Author

I'm gonna go ahead and try getting this written, actually

Done :)

builder.md Outdated Show resolved Hide resolved
builder.md Show resolved Hide resolved
least one `.mustache` template. These are usually named `templates` on all
base16 template repositories.

`SCHEME-FILE` being one or more `.yaml` scheme file. The main source for these
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member Author

@Misterio77 Misterio77 Jun 27, 2022

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.

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 schemes/*.yaml schemes2/*.yaml
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
  • Templates files specified by the previous yaml
  • All files specified by SCHEME-FILE

Copy link
Member Author

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.

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?

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?

Copy link
Member

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.

Copy link
Member

@belak belak Jul 1, 2022

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

@Misterio77 Misterio77 Jul 1, 2022

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):

for template in templates
    subtemplates = read_and_parse(template/config.yaml)
    for subtemplate in subtemplates
        for scheme in schemes
            build(subtemplate, scheme)

So, no directory listing, no download stuff, etc.

builder.md Outdated Show resolved Hide resolved
builder.md Outdated Show resolved Hide resolved
the specified `TEMPLATES-DIRECTORY`), the builder MUST iterate through all the
specified schemes and output matching files.

The built filenames MUST be relative to the directory where the command is
Copy link
Member

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).

Copy link
Member Author

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?

Copy link
Member

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.

builder.md Outdated Show resolved Hide resolved
builder.md Outdated Show resolved Hide resolved
builder.md Outdated Show resolved Hide resolved
builder.md Outdated Show resolved Hide resolved
builder.md Outdated Show resolved Hide resolved
@Misterio77
Copy link
Member Author

Can't seem to quote the dark comment, so replying here:

Alternatively, because all the schemes are in 1 repo, we could add dark: true to relevant schemes and default to dark: false (or the reverse).

Hmm I think we should fallback to something yeah. But I think having a string variable is preferred to a boolean one, as there's more config files that expect dark/light instead of true/false.

@Misterio77
Copy link
Member Author

Thanks a lot for all the comments!

@belak
Copy link
Member

belak commented Jun 27, 2022

Hmm I think we should fallback to something yeah. But I think having a string variable is preferred to a boolean one, as there's more config files that expect dark/light instead of true/false.

The issue is that mustache doesn't have a method of checking equality (so you couldn't display something if a value is "dark"), but it can check if something is true/false.

See this example from the mustache docs:

{{#repo}}
  <b>{{name}}</b>
{{/repo}}
{{^repo}}
  No repos :(
{{/repo}}

If we set scheme-dark/scheme-dark-mode or scheme-light/scheme-light-mode as a true/false template variable, templates would be able to use that to output different things if it's a dark or light scheme. Otherwise, we'd need a variable for the string light/dark and any other variation that comes up. Maybe we could have a convenience variable for the common case though.

@joshgoebel
Copy link
Contributor

joshgoebel commented Jul 1, 2022

The issue is that mustache doesn't have a method of checking equality (so you couldn't display something if a value is "dark"), but it can check if something is true/false.

Why should this matter? I much prefer an explicit:

mode: light
mode: dark

And the builder can easily expose a boolean is_light and is_dark to the mustache renderer... based on the value of mode... This is no different than all the other auto-generated values we create for mustache to consume.


The core scheme repository was based off the single scheme repository so builders supporting v0.8-v0.9 of the spec can continue to function without changes.
Spec upgrades should be backwards compatible with existing templates and
Copy link
Member

@actionless actionless Jul 1, 2022

Choose a reason for hiding this comment

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

both this and previous version of the spec are breaking this point of the spec itself:

The core scheme repository was based off the single scheme repository so builders supporting v0.8-v0.9 of the spec can continue to function without changes.

#### 3.1.3 CLI example: suggesting user to manually clone the template repo, whilst previous version of the format, backward-compatibility with which is claimed - allowed to automate those steps in a builder via base16-templates-source repo

(more on this is at #3)

i think this should be addressed before releasing the updated spec

Copy link
Contributor

@joshgoebel joshgoebel Jul 1, 2022

Choose a reason for hiding this comment

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

I'm becoming less and less enamored with a builder spec... esp at the level actionless is desiring (and what perhaps we had before)... I think our goal (over time) should be to preserve compatibility with converting schemes into themes using templates generally speaking... not that the exact command line arguments and process for doing so should be set in stone.

I find this an onerous requirement and I honestly don't feel like the project or it's contributors have that kind of time.

I think if someone wants to maintain such a thing they should do it with some small wrapper scripts around one of the official builders... I don't think it needs to be baked into the Base16 spec itself.

Copy link
Member

Choose a reason for hiding this comment

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

I'm becoming less and less enamored with a builder spec... esp at the level actionless is desiring (and what perhaps we had before)... I think our goal (over time) should be to preserve compatibility with converting schemes into themes using templates generally speaking... not that the exact command line arguments and process for doing so should be set in stone.

It's my personal opinion that the spec should be limited to purely the template format and the variables which should be provided. Having the config.yaml and how to handle output is helpful, and might make sense to keep around, but isn't the primary purpose of this spec.

I find this an onerous requirement and I honestly don't feel like the project or it's contributors have that kind of time.

I would agree with this - it's a very large requirement. Additionally, as someone who maintains a builder, I like adding new features and supporting things. Having it this limited removes some of the fun for me.

I think if someone wants to maintain such a thing they should do it with some small wrapper scripts around one of the official builders... I don't think it needs to be baked into the Base16 spec itself.

I think this makes sense. Maybe we could clarify that this spec is focused on a how to build a template rather than provide a complete interface to a CLI builder.

Copy link
Member

Choose a reason for hiding this comment

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

i still think it should be return to the spec but just under new title, "Template management [Optional]" - it never was causing any issues in original base16 builders even when it was required - so i can't imagine how it would hurt when it's optional

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything not in the spec is already optional, by default. 😎 If you want it, built it. 🛠

Copy link
Member Author

@Misterio77 Misterio77 Jul 2, 2022

Choose a reason for hiding this comment

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

I proposed new reference builders be simple and unixy. That is, they shouldn't include the manager feature previous "builders" (quoted as they did more than just build) included.

Copy link
Member Author

@Misterio77 Misterio77 Jul 2, 2022

Choose a reason for hiding this comment

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

Even if this was agreed upon, builders with more features would still be builders and would still be compliant.

I defined what a basic builder feature means, proposed that we create reference builders that implement >just< that feature. Any tool that implements the feature is a superset of that, and still a builder.

This reduces the specification scope, making simpler builders possible by making other features optional.

Copy link
Member Author

@Misterio77 Misterio77 Jul 2, 2022

Choose a reason for hiding this comment

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

I don't think file management should be in the specification. Builders are free to include them, of course, but it makes no sense to standardize something that is targeted at opinionated usage.

Copy link
Contributor

@joshgoebel joshgoebel Jul 2, 2022

Choose a reason for hiding this comment

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

accidentially removed f

It wasn't accidentally removed. It was removed with intention, but the fact that a very few people actually use the "build world" use case wasn't considered... so now the question is what to do now... just because something was once in the spec doesn't mean it always should be.

Old builders will still build.

That's a good point, if @actionless wants the old behavior they can use an older builder (for now)... but I'm in total agreement with you that "build world" should be built on smaller pieces... and it may not even be our responsibility to build/spec it if it's only needed by a few people, as an edge case.

Copy link
Member

@actionless actionless Jul 2, 2022

Choose a reason for hiding this comment

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

I don't think file management should be in the specification. Builders are free to include them, of course, but it makes no sense to standardize something that is targeted at opinionated usage.

It was removed with intention

and why you think you have a better overview on this question than people who previously implemented that feature in original base16 and succesfully maintaining the project for yearS?

the reason why base16 project decayed because original maintainer was the only owner (and during last years was not active which lead to raising amount of often trivial PRs like adding new templates/schemes to index), not because it was numerous complains of spec being super-complicated because templates are inside yaml index

your arguments and motivation are like if i proposing a new feature, while i'm proposing to fix thing which was working for years and just got removed for no reason - deliberaly breaking backward-compatibility requirement of a spec

you failing to explain any real reasons to remove it - only different phrases which all rewording in different order what you dont understand why other people are using it, while you are not using it

just because something was once in the spec doesn't mean it always should be.

i'm quite sure what backward-compatibility offered at the last lines of the spec - means exactly opposite to what you say

@actionless
Copy link
Member

actionless commented Jul 1, 2022

(sorry for jumping in so late - i've noticed only today what i dont have notifications turned on for this repo)

@belak
Copy link
Member

belak commented Jul 1, 2022

Why should this matter? I much prefer an explicit:

mode: light
mode: dark

And the builder can easily expose a boolean is_light and is_dark to the mustache renderer... based on the value of mode... This is no different than all the other auto-generated values we create for mustache to consume.

I don't have strong opinions on this, as long as we also provide something like is_light and is_dark.


- `scheme-name` - obtained from the scheme file
- `scheme-author` - obtained from the scheme file
- `scheme-slug` - obtained from the scheme filename, as described above
- `scheme-slug-underscored` - same as `scheme-slug`, but with dashes replaced with underscores
- `scheme-kind` - either "light" or "dark", calculated from the `base00` color <!-- TODO: This is a candidate for inclusion, let me know your thoughts -->
Copy link
Member

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

Copy link
Contributor

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?

Copy link
Member

@actionless actionless Jul 1, 2022

Choose a reason for hiding this comment

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

means:

  1. if scheme written for previous version of spec:
    1.1) so it doesnt have scheme-kind field defined
  2. and if template written for the current version of spec
    2.1) so it have scheme-kind field used
  3. then builder must compute fallback value for scheme-kind field in such case
    3.1) so spec must define the logic (algorithm) for such computation

Copy link
Member

@actionless actionless Jul 1, 2022

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"

Copy link
Contributor

Choose a reason for hiding this comment

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

calculated from the base00 color

Yes, I assume the spec would specify how this happens mathematically - no argument... there are existing luminescence formulas we should probably use.

Copy link
Member

@actionless actionless Jul 1, 2022

Choose a reason for hiding this comment

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

there are existing luminescence formulas

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

Copy link
Contributor

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.

@Misterio77
Copy link
Member Author

Misterio77 commented Jul 1, 2022

I'm a bit lost in the new discussions, as I'm on my semester finals and having trouble to keep up.

But I just wanted to clear up that my rationales with the change are:

  • Avoiding proliferations of fundamentally different "builders" (that currently do much more than just build)
  • Providing one or more official script-friendly builder CLI, without feature creep, state, or implicit behaviour. Scripting with a tool that manages a directory of or vendors schemes is a nightmare, at least in my vision.
  • Helping the ecossystem evolve by providing high-quality library bindings, allowing us to better enforce compatibility, avoiding code duplication and random per-application extensions, etc.
    • Reference builders would be simple to write and maintainable, allowing us to keep everything going smooth
    • Anyone writing stuff that uses base16 would just have to use a library and write their own functionalities, not implement the spec over and over

If the majority thinks this is too much opinionated and will do more harm than good, I'll gladly just clarify the current template behaviour and remove the builder part

@joshgoebel
Copy link
Contributor

Avoid... proliferation ..."builders" (that ... do much more than just build)

Yes, please... builders should build... simpler is better than complex.

official script-friendly builder CLI, without feature creep, state, or implicit behaviour.

I see the use case, just don't feel it needs to be part of the official spec. We should describe the formats (input/output) more so than the tooling itself IMHO. We have a whole official repository to ship a reference CLI - lets show people what it should look like by just building it.

Often building a thing you learn it's not the thing you wanted at all so even if we needed a CLI spec i'd say it would first start with a proposed reference implementation for review.

Helping the ecossystem evolve by providing high-quality library bindings, allowing us to better enforce compatibility, avoiding code duplication and random per-application extensions, etc.

Do we have a compatibility problem that requires "enforcement"? I think you're referring here to should vs must.... I'm going to take another look.

@Misterio77
Copy link
Member Author

Why should this matter? I much prefer an explicit:

mode: light
mode: dark

And the builder can easily expose a boolean is_light and is_dark to the mustache renderer... based on the value of mode... This is no different than all the other auto-generated values we create for mustache to consume.

I don't have strong opinions on this, as long as we also provide something like is_light and is_dark.

Agreed.

If we keep reference builders small and focused, adding more template variables shouldn't be an issue. I think it would be really nice to have scheme_type (that could optionally be provided by the scheme, or calculated by the builder based on base00 luminance) as well as is_dark and is_light.

@Misterio77
Copy link
Member Author

Misterio77 commented Jul 1, 2022

official script-friendly builder CLI, without feature creep, state, or implicit behaviour.

I see the use case, just don't feel it needs to be part of the official spec. We should describe the formats (input/output) more so than the tooling itself IMHO. We have a whole official repository to ship a reference CLI - lets show people what it should look like by just building it.

Often building a thing you learn it's not the thing you wanted at all so even if we needed a CLI spec i'd say it would first start with a proposed reference implementation for review.

Yup, I'm working on a rust builder implementing this spec as a demo, but I'm a little busy IRL right now. But should be ready soon!

Helping the ecossystem evolve by providing high-quality library bindings, allowing us to better enforce compatibility, avoiding code duplication and random per-application extensions, etc.

Do we have a compatibility problem that requires "enforcement"? I think you're referring here to should vs must.... I'm going to take another look.

I think enforcing wasn't exactly the right word (not my first language, sorry), but I mean more like providing an abstraction for people building base16-compatible software! When using a reference builder as a library, they wouldn't have to worry, for example, implementing new template variables we come up with! They would just focus on their own features (managing and installing stuff, etc).

@belak
Copy link
Member

belak commented Jul 1, 2022

Ok. It seems like there are 2 changes in this PR - clarifying the template language and defining a scriptable builder spec. Let's split those out into 2 proposals/PRs - the template spec (with input/output types) will be this spec and we'll add a new (optional) stable CLI spec meant for building tooling on top of.

We can start with the template changes (and maybe make that 0.10.1) and add the CLI separately, maybe as 0.11.0.

@Misterio77
Copy link
Member Author

Sounds good! :)

Thanks for the work, @belak. I'm gonna go ahead and close this PR to reopen separate ones.

@Misterio77 Misterio77 closed this Jul 1, 2022
@joshgoebel
Copy link
Contributor

Specific thoughts on the oveall spec itself: First of all, great job. There is a TON to like here... I think it's an improvement in many ways. (by being more explicit - though I disagree in some cases)

"To be fully compliant, a builder CLI interface MUST NOT include any other feature, option, argument, or deviance from the expected interface and behavior of the program."

As maintainer of a current builder I dislike this... you want single template, multi-scheme... why? why not single/single (also easily script-able)? I already support multi/multi... must I delete that functionality just to conform to the new spec?

If we end up with the "one true CLI spec" I'd like to better understand how we decided this was the shape of things...


As above, the library MUST implement single feature: building templates.

Oh? Not parsing color schemes or template configs into useful object representations? Every library MUST build? We're right back to my #13 discussion. I would not have guessed this was the primary and ONLY requirement - or maybe I would have - I've always been confused what a library would do (at this point) OTHER than build (or consume colors)...

I see us needing to add some features (#11 to start) to Base16 as this thing grows and having to code and maintain full builders in every different language doesn't sound like the best idea... it sounds like a (still) small project trying to far overreach. Is the organization responsible for all these libraries? Must they then ALL be upgraded before a new spec bump/feature release can happen?

I feel like we should perhaps aim for 'less is more' here... a reference builder, a spec... when the spec and reference builder are in sync we can release... we're not responsible for the world - all the libraries, all the builders, all the tooling, etc...

A builder tool MUST provide the following variables, and no others,

I know why you have "and no others" but this also rubs me the wrong way... I think we should encourage innovation... we already have a GitHub organization to help "steer" the project... if I went off and added #11 to my builder (as a proof of concept) yet it also still built existing themes just fine, I don't think it's quite honest to say it "isn't in spec"...

Perhaps if you added a --strict mode or something... and when in that mode a builder must do exactly as you wish, but in another mode it could "get creative".... that sounds a lot nicer to me.

@Misterio77
Copy link
Member Author

Misterio77 commented Jul 2, 2022

Hey @joshgoebel, thanks for the feedback :)

I think I'm having some trouble getting my point across (this PR being refactored multiple times didn't help either). But the idea is not culling what builders can do, but defining clearly what is required for building a mininal reference builder.

I'm trying to make the specification easier to implement, so that maintaining reference builders binaries (for scripting) and bindings (for other builders to use) with a small team is actually possible. The reference builders should be easily implemented in just a few dozen LOC (possibly without any external dependencies!). While, still letting non-reference (but still compliant!!) builders go wild (extend the standard, add management features, whatever their heart desire).

Builders designed for end user usage should definitely have a more convenient CLI usage (see flavours for an example of what I consider an extended opinionated builder to be).

The idea is building very simple and easy to maintain reference builders that more featureful ones can build upon.

Oh? Not parsing color schemes or template configs into useful object representations? Every library MUST build? We're right back to my #13 discussion. I would not have guessed this was the primary and ONLY requirement - or maybe I would have - I've always been confused what a library would do (at this point) OTHER than build (or consume colors)...

This was actually an oversight while refactoring the PR, sorry. It previously read that libraries should at least offer builder functionality, but definitively expose useful functionality (color representating and manipulating classes, for example).

@Misterio77
Copy link
Member Author

Misterio77 commented Jul 2, 2022

Here we go: #23

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.

6 participants