Skip to content

Conversation

@XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Feb 14, 2021

So, this is a tricky one.

  1. the build errors aren't real errors; the build continued and resulted in wrong highlighting, see for example https://github.com/nodejs/nodejs.org/runs/1896479847#step:6:31. AFAICT this is by design in the metalsmith-prism package: https://github.com/Availity/metalsmith-prism/blob/8db23a433d600b3fd13e33779cbd264e053d7caf/lib/index.js#L41-L47
  2. maybe this should be implemented in the changelog maker or something?

Before:

image

After:

image

Any of these should do the job: shell-session, sh-session, shellsession

@targos
Copy link
Member

targos commented Feb 14, 2021

@XhmikosR
Copy link
Contributor Author

Is the rule to always use shell-session instead of console ?

Any of shell-session, sh-session, shellsession will work with prismjs, but yeah, this isn't what GitHub's syntax highlighting works with.

Hence why I think there should be some kind of enforcement for this specific case which we have hit more than once (#2923)

@XhmikosR
Copy link
Contributor Author

The other options would be

  1. ask upstream in prismjs to add an alias for this
  2. see if we can do this manually
  3. switch to another solution altogether

@targos
Copy link
Member

targos commented Feb 14, 2021

Maybe we can do something in the release post script to transform occurences of ```console to ```shell-session ?

@XhmikosR
Copy link
Contributor Author

Yup, that could work, but I'm not sure I have the time to propose a patch :)

That being said, a fix there along with a test case should be a pretty good fix.

@XhmikosR XhmikosR merged commit 9e9b463 into master Feb 14, 2021
@XhmikosR XhmikosR deleted the XhmikosR-patch-3 branch February 14, 2021 18:16
SEWeiTung added a commit that referenced this pull request Feb 23, 2021
"Metal-Prism" needs a range of coding languages to be rendered with,
however the script will generate some languages that don't match what it
really needs. This is a fixture for it.

For more, please see: #3695.
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.

4 participants