Skip to content

feat: add displayName to BUNDLED_LANGUAGES#470

Merged
octref merged 26 commits into
shikijs:mainfrom
lawvs:feat/display-name
May 28, 2023
Merged

feat: add displayName to BUNDLED_LANGUAGES#470
octref merged 26 commits into
shikijs:mainfrom
lawvs:feat/display-name

Conversation

@lawvs

@lawvs lawvs commented May 12, 2023

Copy link
Copy Markdown
Contributor
  • I have read docs for adding a language.
  • I have searched around and this is the most up-to-date, actively maintained version of the language grammar.
  • I have added a sample file that includes a variety of language syntaxes and succinctly captures the idiosyncrasy of a language. See docs for requirement.

@netlify

netlify Bot commented May 12, 2023

Copy link
Copy Markdown

Deploy Preview for shiki-matsu ready!

Name Link
🔨 Latest commit 79fb759
🔍 Latest deploy log https://app.netlify.com/sites/shiki-matsu/deploys/6473bc5e51ec760008b8cf46
😎 Deploy Preview https://deploy-preview-470--shiki-matsu.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@lawvs
lawvs force-pushed the feat/display-name branch from bd0148e to f321a53 Compare May 12, 2023 09:51
@lawvs

lawvs commented May 13, 2023

Copy link
Copy Markdown
Contributor Author

@octref I directly modified the name field (keep it as it is) in the grammar files and am uncertain whether this change will have any unintended consequences. Should I instead add a new displayName field to avoid any issues?

Note some files may not have a name field, such as apl, cadence, fsharp.

@lawvs
lawvs marked this pull request as ready for review May 13, 2023 18:53
Comment thread packages/shiki/src/languages.ts Outdated

@octref octref left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a great start! As you noticed, some name in the original grammar doesn't make sense, like MagicPython, or ASP vb.NET.

Here's what I'd like to see:

  • Keep the original match between the normalized grammar filename & the grammar's name field. For example, x.tmLanguage.json should have name as x, in the same case
  • Add another variable like vscodeGrammarDisplayNames in grammarSources.ts. When importing grammars from VS Code, the import script should follow this mapping to find out the correct displayName.
  • For githubGrammarSources, change the signature to { id: string, displayName?: string, source: string }[].

Good work and you are really close! Thanks.

@lawvs
lawvs force-pushed the feat/display-name branch 2 times, most recently from 35a8a17 to 0b6c037 Compare May 26, 2023 20:30
@lawvs
lawvs requested a review from octref May 26, 2023 21:15
@octref

octref commented May 27, 2023

Copy link
Copy Markdown
Collaborator

close/open to trigger ci

@octref octref closed this May 27, 2023
@octref octref reopened this May 27, 2023
}
}
},
"scopeName": "text.tex"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I shouldn't have made changes to these files (languages/*.json), but they were changed after running the update:grammars script.

}
}

contentObj['name'] = name

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The name field will be normalized in the scripts/grammars/normalizeGrammarPaths.ts, so removing it is safe.

@octref
octref force-pushed the feat/display-name branch from 00027a5 to 1f04871 Compare May 28, 2023 17:15

@octref octref left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey, really good work! I have some minor adjustments, mostly for consistency, so I made the changes and pushed them. Thanks a lot for your contribution 🚀

Comment thread scripts/grammars/collectDisplayName.ts Outdated
Comment on lines +27 to +30
if (!fs.existsSync(fPath)) {
console.log(`${chalk.red("'file not found in")} ${chalk.blue(f)}`)
return
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this necessary? You just read all the files in the dir so they should all exist

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.

2 participants