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 solidity language #3973

Merged
merged 11 commits into from
Jan 12, 2018
Merged

Add solidity language #3973

merged 11 commits into from
Jan 12, 2018

Conversation

oktapodia
Copy link
Contributor

@oktapodia oktapodia commented Jan 5, 2018

The main reason there's still no Solidity syntax highlighting on GitHub is that the .sol extension is too generic.

Then, following this thread, having the extension .solidity for now and use the .gitattributes file to set manually the .sol extension on our side is the easiest way for now

Current public usage is over 400k hits, see here and here

Solidity is actively developed, used to manage the ethereum smart contract and it has been published by Ethereum.

Closes #3755 as superceded. PR #3755 fails CI due to a color proximity warning.
The sample file comes from OpenZeppelin and is licensed under the MIT license. The file has been edited to reflect the fact.

@oktapodia oktapodia changed the title [WIP] Solidity2 updated Add solidity language Jan 9, 2018
@oktapodia
Copy link
Contributor Author

Fix ethereum/solidity#2681

@oktapodia
Copy link
Contributor Author

@pchaigno can you review it, please?

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I do, however have several points:

  • You've not provided a description for this PR. Please do so, but read the rest of these points first as you can address some of them in your description.
  • This PR appears to be a duplicate of Add the Solidity language without an associated extension (updated) #3755 which you've commented on. Why the duplication and not the continuation of that PR?
  • Proposal: use and recommend .solidity file extension (for syntax highlighting on github) ethereum/solidity#2681 hasn't been merged. Has the proposal been accepted by the upstream project? If so, please provide a link to the search results showing the adoption on GitHub.
  • Please re-state the original source and licenses of the sample files included in this PR. Yes, this is stated in the original PR, but if you're intending for this PR to replace that one, we'd like the source and license of the sample files noted in the PR adding them as it's not clearly stated within the files themselves.
  • The two .solidity files need to be moved to samples. The previous suggestion of moving them to test/fixtures was because that PR was attempting to add support for Solidity without an extension. This PR is providing support with an extension so normal locations apply.
  • We prefer samples of real in-the-wild usage of the language. pygments-example.solidity is clearly only designed for the syntax highlighting and not the language. Linguist doesn't do the actual syntax highlighting, so this file doesn't really belong in this repo. Please remove it, and ideally, though not necessary, add another example of real-world usage.
  • On the topic of syntax highlighting... you've not added a syntax highlighting grammar. Looks like https://github.com/caktux/language-ethereum may do the trick, so you may want to add that as part of this PR so peeps get language recognition and pretty colours.

@veox
Copy link

veox commented Jan 9, 2018

Note that this PR (#3973) is the 7th in the Solidity series.

PR #3755 is the 6th. PR #3560 was the 5th, and has a reference for earlier ones.

No, it hasn't been accepted, neither by the team maintaining the compiler, nor the community at large. (As a social-first approach to issue resolution, it may never come to fruition; and that is fine.)

  • On the topic of syntax highlighting... you've not added a syntax highlighting grammar.

davidhq/SublimeEthereum is already present in .gitmodules and grammars.yml (links to current master HEAD). See PR #3161 (the grammar was added manually).

@oktapodia
Copy link
Contributor Author

oktapodia commented Jan 9, 2018

Thanks for your answer @lildude

  • Sorry about the description, I updated it!
  • I based my pull request on is fork, then rebased, then pushed into my repository (I could not update his PR because I don't have a write access to his repository)

@veox already answered many of your questions and I think the syntax highlighter is already here

@oktapodia
Copy link
Contributor Author

oktapodia commented Jan 9, 2018

I added a comment to ask one of the solidity members to approve it here

@veox
Copy link

veox commented Jan 9, 2018

@oktapodia The sample you've included is from ethereum/solidity, which falls under GPLv3 AFAIK.

May I suggest something from OpenZeppelin/zeppelin-solidity, which is under an MIT license?.. In particular, OZ token implementations are quite rampant. Careful with the token ones: some may include prior work from FirstBlood, which seems to be lacking a license of any kind.

@oktapodia
Copy link
Contributor Author

@veox sure, send me the link of the example you prefer and I will add it :)

@lildude
Copy link
Member

lildude commented Jan 9, 2018

One is already present in .gitmodules (link to current master HEAD) - davidhq/SublimeEthereum.

Ah, I missed that as it isn't currently referenced in the vendor/README.md, but this PR now updates that. 🙇.

@veox
Copy link

veox commented Jan 9, 2018

@oktapodia You have previously included a RefundVault.sol from OZ, which IMO is a good fit.

(The file does not have a license header, so you'll need to include a statement in the OP.)

@oktapodia
Copy link
Contributor Author

@veox Done

@veox
Copy link

veox commented Jan 9, 2018

@oktapodia It would be nice if you addressed @lildude's points in the PR summary. Adding something like:

Link helper: RefundVault.sol and LICENSE.

@veox
Copy link

veox commented Jan 9, 2018

@oktapodia The blob links I've suggested are for the latest current RefundVault.sol. The file included as part of this PR seems to be for an earlier version.

@veox
Copy link

veox commented Jan 9, 2018

@lildude:

The two most pressing issues from your list - lack of license statement for the sample file, and missing reference to existing PR - have been addressed in the PR summary.

The rest have been addressed in comments.

@lildude
Copy link
Member

lildude commented Jan 9, 2018

The two most pressing issues from your list - lack of license statement for the sample file, and missing reference to existing PR - have been addressed in the PR summary.

The rest have been addressed in comments.

Perfect! Thanks. I really appreciate your work.

I prefer to give my final sign off once one of the non-GitHub maintainers has reviewed and given their approval.

@oktapodia
Copy link
Contributor Author

Thanks @veox for your help!

@lildude
Copy link
Member

lildude commented Jan 12, 2018

Approving now so when @pchaigno or @Alhadis get around to reviewing, they can merge without waiting for me to unblock things.

lildude
lildude previously approved these changes Jan 12, 2018
@lildude
Copy link
Member

lildude commented Feb 8, 2019

Something to do with the recent release?

Oooof, yes.

I released Linguist v7.2.0 this morning which included an update to all grammars including SublimeEthereum which is used for Solidity highlighting, but now I look at it, I can see the problem:

screenshot 2019-02-08 at 13 25 43

The grammar has switched from a TextMate compatible grammar to a Sublime Text compatible grammar - Linguist does not support .sublime-syntax grammars:

https://github.com/github/linguist/blob/e4560984058b4726010ca4b8f03ed9d0f8f464db/tools/grammars/compiler/loader_fs.go#L18-L23

This wasn't picked up when I built the release because unsupported formats are silently ignored as many grammars include both Sublime and TextMate compatible grammars.

@veox
Copy link

veox commented Feb 8, 2019

The last tagged release of davidhq/SublimeEthereum to have .tmLanguage is v1.0.3.

Ping @davidhq: see comment above: .sublime-syntax is not supported. Will also open issue in the repo.


git submodule allows tracking a particular git branch, e.g. (in .gitmodules):

[submodule "vendor/grammars/SublimeEthereum"]
	path = vendor/grammars/SublimeEthereum
        url = https://github.com/davidhq/SublimeEthereum.git
        branch = linguist

Provided @davidhq can create that branch, and push/reset it to v1.0.3, that could work as a stop-gap measure. But it would require a new linguist release. :/


Alternatively, the file could be re-added on master.

@veox
Copy link

veox commented Feb 8, 2019

@lildude Are there any other grammars besides Solidity that are now silently ignored?..

There were no other .tmLanguage files in PR #4408 that were outright shredded.

(junk part removed)

@lildude
Copy link
Member

lildude commented Feb 8, 2019

@lildude Are there any other grammars besides Solidity that are now silently ignored?..

I've just double-checked and it's not actually failing silently... it's just lost in the other noise caused by the many known grammar issues as documented in #3924 which we ignore.

Running it again, I've just spotted:

- [ ] repository `vendor/grammars/SublimeEthereum` (from https://github.com/davidhq/SublimeEthereum.git) (1 errors)
    - [ ] Missing scope in repository: `source.solidity` is listed in grammars.yml but cannot be found

Looking through that output, no other grammars are reporting this that are not already known, so I don't think anything else has slipped past.

@davidhq
Copy link
Contributor

davidhq commented Feb 8, 2019

Hi, I couldn't figure out if Linguist supports .sublime-syntax or not and indeed I removed .tmLanguage and was waiting for a report like this one :/ sorry about that... we can solve as you suggest - both options... what would be the best? I'd rather not add it back to master because of possible confusion... you suggested branch "linguist" in SublimeEthereum repo, I created it: https://github.com/davidhq/SublimeEthereum/tree/linguist ... does this remedy the issue for now?

@davidhq
Copy link
Contributor

davidhq commented Feb 8, 2019

of course .tmLanguage was removed because of rewrite... also wasn't sure if all versions of SublimeText would pick up the correct syntax if both were included...

let me know if for now I can help with anything else

@davidhq
Copy link
Contributor

davidhq commented Feb 8, 2019

PS: davidhq/SublimeEthereum#29 ← any tips here would be welcome

@pchaigno
Copy link
Contributor

@lildude Should we do a quick release to fix this?

@lildude
Copy link
Member

lildude commented Feb 11, 2019

@lildude Should we do a quick release to fix this?

I'll try squeeze it in sometime this week.

@davidhq
Copy link
Contributor

davidhq commented Feb 20, 2019

@lildude .sol still broken

@lildude
Copy link
Member

lildude commented Feb 20, 2019

@lildude .sol still broken

Still haven't had time to make a new release 😉

@dmdque
Copy link

dmdque commented Feb 25, 2019

I'm hoping you can get it in soon, since it's making reading Solidity code difficult across all projects in the Ethereum ecosystem.

jeremyschlatter added a commit to reserve-protocol/rsd that referenced this pull request Mar 2, 2019
GitHub's Solidity highlighting is currently broken site-wide, but will
probably be fixed soon. Tracking issue:

github-linguist/linguist#3973 (comment)

Because of the site-wide issue, this won't work right away, but hopefully will
make our repo ready for highlighting when the issue is fixed.
@dmdque
Copy link

dmdque commented Mar 6, 2019

I noticed both PRs were merged 🎉 but syntax highlighting is still broken. Any idea when the changes will take effect?

@lildude
Copy link
Member

lildude commented Mar 7, 2019

@dmdque same as usual... when I make a release 😀. As you've seen, I've been working on a few improvements this week related to the grammar compiler so unexpected upstream changes outside of the control of Linguist are less likely to slip through the net in future. I'm not quite finished, but when I am, I'll make a release. My aim is for next week.

@davidhq
Copy link
Contributor

davidhq commented Mar 7, 2019

I would say this is great ;) In fact I expected this to already be the case and that it wouldn't be possible to break syntax for anything over entire GitHub... 'Resilience' is very important.

When it's finished and in particular the solidity syntax is back online, we can perhaps talk some more on how to bring separate project branch linguist will be using more up to date with latest version which is improving every day - one that uses stack based .sublime-syntax... If this is not planned to be supported inside linguist anytime soon, I will update the linguist branch with as many recent changes as I can but I won't be able to do a perfect job just with regexes :/ As said: debate for a bit later, for now the priority should rightfully be to improve your release mechanism and point linguist to the linguist branch of SublimeEthereum project when possible.

@lildude lildude mentioned this pull request Mar 12, 2019
@Alhadis Alhadis mentioned this pull request Jul 9, 2019
16 tasks
snowlotus530 pushed a commit to snowlotus530/kleros_ethereum that referenced this pull request Sep 25, 2021
Syntax highlighting on contracts would make it easier to browse contract code in github. 
See: github-linguist/linguist#3973
snowlotus530 pushed a commit to snowlotus530/kleros_ethereum that referenced this pull request Sep 25, 2021
Syntax highlighting on contracts would make it easier to browse contract code in github. 
See: github-linguist/linguist#3973
laion01 added a commit to laion01/kleros that referenced this pull request Oct 26, 2021
Syntax highlighting on contracts would make it easier to browse contract code in github. 
See: github-linguist/linguist#3973
blockchainexpert912 added a commit to blockchainexpert912/krumfi-reserve-protocol that referenced this pull request May 6, 2023
GitHub's Solidity highlighting is currently broken site-wide, but will
probably be fixed soon. Tracking issue:

github-linguist/linguist#3973 (comment)

Because of the site-wide issue, this won't work right away, but hopefully will
make our repo ready for highlighting when the issue is fixed.
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants