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

change transcript info string format #5214

Closed
sellout opened this issue Jul 11, 2024 · 9 comments
Closed

change transcript info string format #5214

sellout opened this issue Jul 11, 2024 · 9 comments

Comments

@sellout
Copy link
Contributor

sellout commented Jul 11, 2024

Right now transcripts use an info string that looks like language:flag:other-flag filename, roughly – e.g. ```unison:added-by-ucm scratch.u.

The first word of the info string is typically used to specify the language of the code sample, and rendered in the class attribute of the code tag. However, this spec does not mandate any particular treatment of the info string.
CommonMark Spec §4.5

The GFM spec says the same thing. Neither seems to define “word”, unfortunately, so it’s hard to say what it means. But, in practice it seems to be basically space-delimited. What that means is that with the current syntax we use in transcripts, we end up with

```unison:added-by-ucm
foo = 2 + 2
``` 

becoming

<pre>
  <code class="language-unison:added-by-ucm">
    foo = 2 + 2
  </code>
</pre>

when it should contain <code class="language-unison">.

Despite GitHub not yet recognizing Unison, other places (e.g., my editor) can, but don’t highlight unison:added-by-ucm blocks, but my editor does highlight ```unison added-by-ucm blocks.

I propose that we change the syntax. The minimal change would be to replace the first : with a space, so ```unison added-by-ucm scratch.u … but there are already cases where we have the file name, but no flags provided, which would be ambiguous. So I wonder if we can come up with a more consistent set of flags that are amenable to Unison adding more in the future.

@sellout
Copy link
Contributor Author

sellout commented Jul 11, 2024

There are related changes suggested in #5199, in terms of what flags should exist.

@sellout
Copy link
Contributor Author

sellout commented Oct 3, 2024

It seems like there’s a light consensus toward something like

```ucm {hide=all}
scratch/main> builtins.merge
```

```unison {error=intentional file="skritch.u"}
foo = bar
```

A starting point for discussing the set of options:

  • result can be applied to any code block (currently api doesn’t support it) and has the values
  • hide can be applied to any code block (currently api doesn’t support it) and has the values
    • none (the default)
    • output
    • all
  • file can only be applied to unison and is an arbitrary string (the quotes are optional, but necessary if there are spaces)
  • generated has a boolean value (default is false) and corresponds to :added-by-ucm

@sellout
Copy link
Contributor Author

sellout commented Oct 7, 2024

In #5394, I added both :failure and :incorrect to the set of info string tags. It made me wonder if perhaps result in the proposal above should be split into two othogonal options:

  • expectError: bool (default is false)
  • isFailing: bool (default is false)

So,

  • {isFailing=true} – stanza currently errors, but shouldn’t be
  • {expectError=true} – stanza is intentionally erroring
  • {expectError=true, isFailing=true} – stanza is expected to error, but currently isn’t
  • {} (or no info attributes) – this is a standard successful stanza

This doesn’t quite mesh with the ignored result type, though – these attributes are just meaningless in that context.

@sellout
Copy link
Contributor Author

sellout commented Oct 7, 2024

To get back to basics, all that is really required for this is inserting a space, so that

``` unison:hide:error file

becomes

``` unison :hide:error file

We can make the space optional to start, which is a trivial change to the parser.

I still think a richer syntax would be good, but not necessary to fix the CommonMark issue.

(This is also another place we could add a warning – that the old syntax is deprecated.)

@sellout
Copy link
Contributor Author

sellout commented Oct 7, 2024

🤦🏼 I just realized that putting a space there is already allowed, just not shown or documented. The docs should definitely prfer the version with a space.

There are still some inconsistencies – like you can have a space in :hide :error in unison, but not in ucm. And you can never have a space in :hide:all. Plus the order is fixed – :hide has to come before :error.

sellout added a commit to sellout/unison that referenced this issue Oct 8, 2024
The bulk of this updates transcripts to put spaces around the language
name in code blocks. E.g.,
```` markdown
```ucm:hide
````
becomes
```` markdown
``` ucm :hide
````

This corresponds to
https://share.unison-lang.org/@unison/website/contributions/11, which
updates the docs in the same way.

This is effectively a fix for unisonweb#5214, but that issue also has good recommendations for future changes to info strings, so
I don’t know that it should be closed.
@aryairani
Copy link
Contributor

aryairani commented Oct 11, 2024

How about
``` unison {shouldError} fails intentionally

``` unison {broken} fails but shouldn't fail

``` unison {shouldError, broken} should fail but doesn't fail
?
or even error / broken

Reading #5199 I wasn't clear on when you'd want ignore (and depending on the answer, whether that could be handled differently)

@aryairani
Copy link
Contributor

@sellout This is done, right?

@sellout
Copy link
Contributor Author

sellout commented Dec 7, 2024

The “necessary” part is done – allowing (and preferring) a space between the language and other tags.

What still is incomplete is

  1. allowing tags to be specified in any order and
  2. possibly changing the format.

The first one probably should just be a separate ticket.

For the second, the motivation is much weaker now than it was before, so probably worth closing and waiting for a better reason to change it (e.g., if some info string structure gets standardized or some tooling has built-in support for some other format).

@sellout
Copy link
Contributor Author

sellout commented Dec 10, 2024

Ok, #5494 now covers allowing tags to be in any order.

And since we don’t have real motivation to change the tag format for now, this can be closed.

@sellout sellout closed this as completed Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants