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

Upgrade rubygem versions. #19716

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Upgrade rubygem versions. #19716

wants to merge 5 commits into from

Conversation

yilinwei
Copy link
Contributor

@yilinwei yilinwei commented Feb 17, 2024

  • This upgrades from ruby 2.7 to 3.3 since 2.7 is eol.
  • Upgrades to jekyll 4.3.3 since 3.x is only supporting maintenance fixes at this point.
  • Drops redcarpet since jekyll has dropped support for it.
  • Moves the dependencies to the Gemfile

Notes

Syntax highlighting

We don't use Jekyll's syntax highlighting (rouge) at all. As far as I can tell this is due to:

  1. It doesn't support EBNF
  2. We do a "two-step" render process where the latex pre-preprocessor spat out the html and highlight.js used to support merging of html nodes.

Because we embed math elements into the snippets - something more complicated like using the tree-sitter-based grammar is a no-go; it is not nearly permissive enough to deal with what we're doing. We could use a strategy like replacing the math elements with placeholder identifiers (making sure to respect Scala's naming rules) and substitute them back in - but treesitter doesn't support EBNF either, which is a large part of the spec.

However since highlight-js has removed support for this feature, we could potentially look at doing this server-side or using rouge. This has the added benefit that it would be more inline with Github's own syntax highlighting I think. Since I don't know ruby or rouge at all, I haven't done a thorough investigation of this option.

I do, on the other hand, know Javascript so I've added started to add the merge behaviour back in like before, so we can now upgrade the highlight.js version successfully.

const start = match.index;
match = re.exec(text);
if(match == null) {
break;
Copy link
Member

Choose a reason for hiding this comment

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

There is a weird mix of tabs and spaces here. The surrounding code uses 2 spaces for indentation of JavaScript code. We should stick to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will - do I am going through each bit of the snippets first to make sure the rendering works from before. I will unmark as draft and untabify once it's done.

Copy link
Member

Choose a reason for hiding this comment

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

Changes in this file should be reflected at https://github.com/lampepfl/dotty/blob/ca88156789b2c94f107f85f07b238a59d84b7d65/.github/workflows/spec.yml#L26-L37 as well. I guess we should add a comment in the current file warning about this.

bishabosha added a commit that referenced this pull request Feb 21, 2024
…thub workflows (#19718)

Currently the local server for the spec fails to build with the same
issue as in #19288 on a fresh
docker build.

This is the minimum amount of changes to actually get the spec building
for users to make changes. I highly recommend updating all the versions
when #19716 is finished.
@bishabosha
Copy link
Member

I've noticed this is based on a very old branch (previous commit is from September 2023), do you mind rebasing?

@bishabosha
Copy link
Member

bishabosha commented Feb 21, 2024

The latex handling also only seems to be working in specific places, e.g. only one E here is substituted correctly:

Screenshot 2024-02-21 at 15 54 19

for comparison with the current deployed docs:

Screenshot 2024-02-21 at 15 56 25

@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 21, 2024

@bishabosha

Yup, I have a local branch which is in-progress and I'll undraft/rebase this once it's done. Basically I don't deal with the case where the backtick extends across multiple highlight spans (I deal with the inverse, where one span contains multiple backtick expansions). Some of issues here are also because it infers the wrong language as well - for some reason it really likes to infer as perl...

@bishabosha
Copy link
Member

bishabosha commented Feb 21, 2024

That is quite tricky. Perhaps your other idea of escaping the backticks at build time makes more sense?

Maybe each backtick is subbed by id___35 (index 35) and then each decoded fragment is stored in a json array possibly

(I have no idea how this works for disclaimer)

@yilinwei
Copy link
Contributor Author

yilinwei commented Feb 23, 2024

@bishabosha

I'd love that to be that simple, but we backtick across delimiters such as T_0, ..., T_n, so there would have to be an adhoc ruleset about how we introduce the backtick segments (i.e. you want id__xx per T) so that the syntax highlighting would still highlight the correct spans. Not saying we couldn't do, just that it isn't completely straightforward, and I would be more in favour of preserving the current behaviour if possible.

I have some time to look at this, this weekend and I think that it's pretty much there on my own local branch.

@bishabosha
Copy link
Member

I have some time to look at this, this weekend and I think that it's pretty much there on my own local branch.

awesome, thanks!

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.

3 participants