html: allow a configurable prefix on CSS class names. #296
Conversation
CSS class names are a global namespace. Without some kind of namespace partitioning, it's entirely possible to have class name collisions and all sorts of chaos as different sources of CSS stomp on each other. Hence, add a new `ClassStyle`, `SpacedPrefixed`, which tells syntact to add a configurable prefix to all of the class names that it uses. This patch breaks the API. Some of the breakage is not strictly necessary (e.g., `ClassedHTMLGenerator::new` could stay as it is, and a `new_with_style` method added). However, some is enforced: `ClassStyle` can no longer be `Copy` because it can now contain a `String`, and so `tokens_to_classed_spans` must now accept a reference. Because this is a breaking API change anyway, I took the opportunity to add `ClassStyle` parameters where relevant, to make the parametricity of `ClassedHTMLGenerator` and `css_for_theme` clear.
This proves to be nicer to use, not least because now a `const` `ClassStyle` can be declared. It's also now `Copy` again. It's still technically an enforced breaking change because of the added lifetime parameter, though I doubt whether any actually extant code would be broken. On that basis I've kept the unforced breaks as well, since I do think they add value (primarily, misuse resistance).
|
Having slept on this and used it in anger, I've altered the API changes a bit - there's still a technical, enforced breakage, but much smaller than before. The commit message explains a little bit more. |
There was a problem hiding this comment.
Seems like a good idea, thanks for this! I'm somewhat reluctant to release a full breaking release right now because this would be the only breaking change, and it's in a part of the API that only uses public functionality so if people really need this in the mean time they can always copy parts of the html module into their own codebase and modify it.
I'm happy to just leave this PR sitting here and maybe rebase and do any fixing myself when I want to do the next major release. Alternatively there's a version of this change with only technical breakage that I would accept right now, which is to take out the added style parameter (which I agree is good, just a breakage) and change the lifetime on the string to &'static str instead of &'a str which will remove the lifetime parameter at the cost of only being able to use string literals for the prefix (which I think is all anybody will want). Then it would only break anybody matching on ClassStyle which I don't see why they would ever do since there's currently only one variant.
src/html.rs
Outdated
| #[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
| pub enum ClassStyle { | ||
| pub enum ClassStyle<'a> { |
There was a problem hiding this comment.
While you're at it could you add the new #[non_exhaustive] attribute here so adding new things isn't technically a breaking change (although unlikely to break anything in practice) anymore?
src/html.rs
Outdated
| s.push_str(" ") | ||
| } | ||
| s.push_str(atom_s); | ||
| match style { |
There was a problem hiding this comment.
Instead of putting the match statement on the outside and duplicating the loop code in each branch, could you put it around the s.push_str(&prefix) and just have the ClassStyle::Spaced branch be ()? It should be a very predictable branch so shouldn't have a noticeably performance cost.
src/html.rs
Outdated
| } | ||
|
|
||
| fn scope_to_selector(s: &mut String, scope: Scope, style: ClassStyle) { | ||
| match style { |
There was a problem hiding this comment.
Same thing about pushing the match statement down here.
This removes the enforced API breaks altogether. The unforced breakage is also reverted; now, the methods that assumed `Spaced` are deprecated, and new parameterised ones are provided.
If this turns out to be undesirable for any putative future class styles, it can always be reverted.
|
Yeah, makes sense. I've gone ahead and rolled back all the breaking changes (except for the new constructor, which is the most technical of technical breaks), instead deprecating the non-parameterised versions. |
|
CI keeps being broken even when I retry it, at a stage before it seems to run any of my code. Travis has broken in this way before but fixed itself, but it's been much longer this time so maybe it won't. I ran the tests for this PR locally instead and they passed. |
|
I released |
CSS class names are a global namespace. Without some kind of namespace
partitioning, it's entirely possible to have class name collisions and
all sorts of chaos as different sources of CSS stomp on each other.
Hence, add a new
ClassStyle,SpacedPrefixed, which tells syntactto add a configurable prefix to all of the class names that it uses.
This patch breaks the API. Some of the breakage is not strictly
necessary (e.g.,
ClassedHTMLGenerator::newcould stay as it is, anda
new_with_stylemethod added). However, some is enforced:ClassStylecan no longer beCopybecause it can now contain aString, and sotokens_to_classed_spansmust now accept areference. Because this is a breaking API change anyway, I took the
opportunity to add
ClassStyleparameters where relevant, to make theparametricity of
ClassedHTMLGeneratorandcss_for_themeclear.