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 Cairo language support #5751

Merged
merged 5 commits into from
Feb 23, 2022
Merged

Add Cairo language support #5751

merged 5 commits into from
Feb 23, 2022

Conversation

0xChqrles
Copy link
Contributor

@0xChqrles 0xChqrles commented Jan 28, 2022

Description

Checklist:

@0xChqrles 0xChqrles requested a review from a team as a code owner January 28, 2022 21:21
@lildude
Copy link
Member

lildude commented Feb 2, 2022

Closing as this is a duplicate of #5661

@lildude lildude closed this Feb 2, 2022
@0xChqrles
Copy link
Contributor Author

Hello, I though it would be better to open a new pull request since #5661 is irrelevant, there is no grammar in the added repo, even the "syntax highlighting grammar" link is empty

@lildude
Copy link
Member

lildude commented Feb 2, 2022

Oh yes, you're right. I've not look at that PR yet since I requested the grammar be changed. Maybe you and @rajivpo can work together to merge your two together? (We don't need the samples from both).

I plan to review PRs in the next week or two so if no action has been taken on #5661 by then, I might take this PR instead.

Re-opening to put back onto my radar.

@lildude lildude reopened this Feb 2, 2022
@0xChqrles
Copy link
Contributor Author

Ok thanks, I'll try to contact @rajivpo

@Nixinova Nixinova mentioned this pull request Feb 8, 2022
4 tasks
@Alhadis Alhadis changed the title Add language cairo Add Cairo language support Feb 10, 2022
@davidhq
Copy link
Contributor

davidhq commented Feb 10, 2022

As pointed out to me in #5661 the grammar here isn't an actual TextMate-compatible grammar. #5661 is ready to merge, so if you're happy with it @rajivpo, we can close this PR in favour of that one?

Hi! Copied above from the other thread... so can GitHub (linguist) understand cson natively ? I believe it does but is not clear, so would be good to know for sure.

@xshitaka I was looking around and noticed there is no cairo support for SublimeText yet so I took your grammar and tried to convert to Cairo.tmLanguage for it to work in Sublime. I succeeded but see some mistakes, not sure if they are in original as well or not, was planning to improve the grammar for fun. Do you have plans to make a package for SublimeText as well? I'm currently doing this just for myself to have the syntax and if nobody releases a package, I can then just include it in https://github.com/davidhq/SublimeEthereum which has Solidity and Vyper.. would probably not release a StarkNet package (which would include cairo grammar). If you want to do SublimeText package, even better, let me know what is best. Would probably be great if one of us did it for best results and less fragmentation... otherwise someone else can notice it is missing and release something out of sync with whatever GitHub will use (your repo probably).

So this discussion is only relevant in regard to GitHub in case it cannot actually read cson, then I can help with converted file and @xshitaka can make a new repo in preparation for SublimeText package for example.

Let me know if I can be of any help, thank you.

@0xChqrles
Copy link
Contributor Author

Hi ! @davidhq I've deduced that GitHub does support CSON since some languages already supported by GitHub are using a CSON grammar. Rust for exemple.

About the SublimeText package, feel free to contact me on discord: Ashitaka#7787 if you want to discuss about it outside of this discussion.

@davidhq
Copy link
Contributor

davidhq commented Feb 10, 2022

Hi ! @davidhq I've deduced that GitHub does support CSON since some languages already supported by GitHub are using a CSON grammar. Rust for exemple.

About the SublimeText package, feel free to contact me on discord: Ashitaka#7787 if you want to discuss about it outside of this discussion.

Yes, does support, also https://github-lightshow.herokuapp.com/ supports cson it seems.

ok then, we talk more on Discord and hopefully Cairo is also part of linguist on next update :}

@lildude
Copy link
Member

lildude commented Feb 11, 2022

Yes, does support, also https://github-lightshow.herokuapp.com/ supports cson it seems.

Yup, definitely does… we convert all formats to JSON using this code.

ok then, we talk more on Discord and hopefully Cairo is also part of linguist on next update :}

Better be quick 😉. I'm aiming to release next week or the week after at the latest depending on my work load.

@davidhq
Copy link
Contributor

davidhq commented Feb 11, 2022

ok then, we talk more on Discord and hopefully Cairo is also part of linguist on next update :}

Better be quick 😉. I'm aiming to release next week or the week after at the latest depending on my work load.

If grammar https://github.com/xshitaka/atom-language-cairo remains in consistent working state (as it is now), then it's good enough for inclusion. We talked with @xshitaka yesterday and tested some more in both Sublime and Atom and it looks very suitable for inclusion as a first version that can later be further improved in places.

I'd just suggest that if @xshitaka does any minor improvements this or next week to do it on a separate branch (or locally) and only merge after testing with https://github-lightshow.herokuapp.com/.

@0xChqrles
Copy link
Contributor Author

I'd just suggest that if @xshitaka does any minor improvements this or next week to do it on a separate branch (or locally) and only merge after testing with https://github-lightshow.herokuapp.com/.

Indeed, even if the actual version is suitable for inclusion, there's still a room from improvement, so I'll create another branch for future updates 👍

@0xChqrles
Copy link
Contributor Author

Hi @lildude ! I've seen that you've made a new release yesterday, any news about this PR?

@lildude
Copy link
Member

lildude commented Feb 22, 2022

I was waiting on you and/or @rajivpo and/or @davidhq to confirm which PR (this one or #5661) was going to be used and when it was ready.

I didn't get any confirmation in the ~10 days since I said be quick so I left it out.

@0xChqrles
Copy link
Contributor Author

Sorry, I didn't understand that, it's this PR which will be used, the #5661 one looks dead and anyway it doesn't even add a grammar.
But this one is ready to be released

@0xChqrles
Copy link
Contributor Author

I thought you were telling us to be quick if we wanted to modify the grammar before the release

@davidhq
Copy link
Contributor

davidhq commented Feb 23, 2022

I also thought it was clear this is ready... but misunderstandings happen.
Maybe it can get merged now so this is settled? From there it looks like it will have to wait until next linguist release (unfortunate but factual).

@lildude
Copy link
Member

lildude commented Feb 23, 2022

Thanks for confirming. I've closed #5661 in favour of this one.

I'll merge now, but as pointed out, this won't be available on GitHub until the next release which I aim to make in a month or two's time, though this is subject to change.

@lildude lildude merged commit 6b02d3b into github-linguist:master Feb 23, 2022
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.

None yet

3 participants