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

Testing Nickel snippets in the manual #1611

Merged
merged 9 commits into from
Sep 19, 2023
Merged

Testing Nickel snippets in the manual #1611

merged 9 commits into from
Sep 19, 2023

Conversation

vkleen
Copy link
Contributor

@vkleen vkleen commented Sep 16, 2023

Add a new test suite in core/tests/manual which parses the Markdown files in doc/manual to extract all Nickel code blocks and checks that they parse or evaluate correctly. For now, REPL sessions in the manual aren't evaluated during testing but only parsed.

To facilitate this, code blocks in the manual are annotated with nickel, nickel-lines, nickel-parse, nickel-repl or nickel-no-check depending on the parsing or evaluation mode desired.

Closes #1154

@github-actions github-actions bot temporarily deployed to pull request September 16, 2023 08:17 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 13:11 Inactive
vkleen added a commit to tweag/nickel-lang.org that referenced this pull request Sep 18, 2023
This makes Prismjs understand the new code block annotations needed for
tweag/nickel#1611
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 13:27 Inactive
github-merge-queue bot pushed a commit to tweag/nickel-lang.org that referenced this pull request Sep 18, 2023
This makes Prismjs understand the new code block annotations needed for
tweag/nickel#1611
@vkleen vkleen marked this pull request as ready for review September 18, 2023 14:43
@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 14:54 Inactive
doc/manual/contracts.md Outdated Show resolved Hide resolved
doc/manual/contracts.md Outdated Show resolved Hide resolved
This line is even more indented.
This line has no more indentation.
"%
This line has no indentation.
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 a little confused by this example. I guess you had to indent it extra so that the final "% will be caught by the parser? But even before this change, the "This line has no indentation" line had indentation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The multiline string parser strips common indentation. So this line doesn't have any indentation in the final string and that's what this example is supposed to illustrate.

doc/manual/typing.md Show resolved Hide resolved
# hide-start
let Port = std.contract.from_predicate (fun value =>
std.is_number value
&& value % 1 == 0
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If value isn't an integer, value % 1 returns the fractional part of value. I suppose that's in line with writing x mod 1 for the residue class of x in $\mathbb R/\mathbb Z$...

@github-actions github-actions bot temporarily deployed to pull request September 18, 2023 16:01 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 19, 2023 09:11 Inactive
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

A good step forward!

I wonder if putting a space after nickel in the textinfo of fenced code block isn't more idiomatic markdown. In particular, according to the common mark spec, only the first word of the textinfo (up to the first space) is considered for highlighting purpose (see eg https://spec.commonmark.org/0.28/#example-112). We adapted the website, but for example I often read the manual inside NeoVim which has syntax coloring for Nickel snippet, and who knows what markdown rendered people will use to read the manual outside of the website. Would that make sense to do:

  • nickel-repl => nickel repl
  • nickel-parse => nickel parse

It could be something even more specific to indicate that it doesn't concern rendering, such as nickel @test.parse-only (this is an absolutely random suggestion, not necessarily a good one)

/// A code block that should only be parsed, not evaluated
Parse,
/// A code block containing a REPL session, e.g.
/// ```nickel repl
Copy link
Member

Choose a reason for hiding this comment

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

The example doesn't check out with practice: it seems that you are requiring a - instead of a space, ie nickel-repl.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I tried what you were suggesting about spaces before. Unfortunately, gatsby doesn't seem to ignore the extra information after nickel. In fact, it seems to remove the space after nickel and try to look for a Prismjs language nickelrepl if given nickel repl in the code block. Relying on that behaviour felt a bit too spooky for me, though...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On the other hand, relying on it and just continuing to check when updating the website should make syntax highlighting work in saner markdown renderers, I guess. It's easy to change back here, so whichever you think would make more sense long term.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and what about using a special character and a space, something like nickel #repl or nickel !repl? Gatsby would parse that as nickel#repl which has almost no chance of conflicting with anything, keeping the alias approach (could even be nickel -repl, even if that feels...slightly wrong for some reason), and such that other markdown parser would be ok with it? I tried it into Neovim and some random HTML markdown renderer extension I have and indeed adding a space preserves highlighting, while the dash doesn't. It seems like it doesn't make a difference for Gatsby in one direction or the other, but preserve other renderers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like nickel #... 👍 I'll make the change here and update the gatsby config on the website.

doc/manual/contracts.md Show resolved Hide resolved
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Whatever approach you choose for the textinfo string, it's not a blocker.

@github-actions github-actions bot temporarily deployed to pull request September 19, 2023 12:48 Inactive
@vkleen vkleen added this pull request to the merge queue Sep 19, 2023
Merged via the queue into master with commit b029613 Sep 19, 2023
4 checks passed
@vkleen vkleen deleted the feat/manual-snippets branch September 19, 2023 13:38
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.

Check evaluation of snippets in the manual
3 participants