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

Add codemirror mode for nim #5269

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add codemirror mode for nim #5269

wants to merge 7 commits into from

Conversation

stisa
Copy link

@stisa stisa commented Aug 24, 2020

  • I have read the Contributor Guide
  • I have updated the changelogs/current_changelog.md file with some information about the change that I am making the appropriate file.
  • I have validated or unit-tested the changes that I have made.
  • I have run through the TEST_PLAN.md to ensure that my change does not break anything else.

This adds a codemirror mode for syntax highlighting of nim
If there's some other plugin mechanism in place to load modes for languages not supported by codemirror please let me now and I'll try to use that.

Screenshot:
Annotazione 2020-08-24 171711

@todo
Copy link

todo bot commented Aug 24, 2020

- Can you have imaginary longs?

// TODO - Can you have imaginary longs?
intLiteral = true;
}
// Zero by itself with no other piece of number.
if (stream.match(/^0(?![\dx])/i)) {
intLiteral = true;


This comment was generated by todo based on a TODO comment in 480b353 in #5269. cc @stisa.

@stisa stisa changed the title Stisa nim mode Add codemirror mode for nim Aug 24, 2020
@willingc
Copy link
Member

Hi @stisa,

We would be happy to add support for nim but taking a different approach than this PR. This PR adds Go support and outlines our preferred approach.

We recommend that you create an external package likely using much of the code in this PR.

Thanks! Let us know if you have questions. cc/ @captainsafia

@stisa
Copy link
Author

stisa commented Aug 25, 2020

Hi! Thanks for the reply.
Makes sense, I'll have a look at doing that.

Basically I need to:

  • create a npm package for the mode
  • add the package to the editor's dependecies (here?)
  • require that in index.tsx

Does that sound correct?

@willingc
Copy link
Member

willingc commented Aug 25, 2020

@captainsafia Please correct me if I'm wrong.

@stisa I don't think you need to add the package to the dependencies. Much like Rust or Go, you would require it in packages/editor/src/index.tsx

@stisa
Copy link
Author

stisa commented Aug 27, 2020

@stisa I don't think you need to add the package to the dependencies. Much like Rust or Go, you would require it in packages/editor/src/index.tsx

@willingc I'm pretty sure it's needed, to my understanding rust and go worked like that because they're already present in codemirror's package (here), they just weren't required by nteract. As my mode is external, a dependency is needed for npm to pull it from the registry.

(sorry if I'm wrong, I don't know much about javascript development)

@willingc
Copy link
Member

Thanks @stisa. Your points make sense to me. @captainsafia thoughts?

@captainsafia
Copy link
Member

Unfortunately, we will need to include it in the package.json since it is no bundled with the default set of syntax highlighters that CodeMirror provides.

I say "unfortunately" because this is one of those things that I'd like to avoid having as a dependency of the package and instead allow people to configure in the desktop app.

I don't have any ideas on how to make that possible now but I am open to hearing ideas from others!

@stisa
Copy link
Author

stisa commented Aug 29, 2020

Yeah I understand adding dependencies just to support a language isn't ideal.

One way to do it could be like with jupyter notebooks, where a special file kernel.js is loaded from the kernelspec folder, eg. in loadMode if no mode is found try looking for a <language>.mode.js in the kernelspec folder (or similar) that provides a register function, but I'm not sure how doable it would be with nteract's architecture.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
workflow:needs-review Needs PR review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants