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 support for Huff #6470

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

Conversation

sambacha
Copy link

@sambacha sambacha commented Jul 9, 2023

♞ Huff

Huff is a low-level programming language designed for developing highly optimized smart contracts that run on the Ethereum Virtual Machine (EVM). Huff does not hide the inner workings of the EVM and instead exposes its programming stack to the developer for manual manipulation.

Huff does not hide the inner workings of the EVM and instead exposes its programming stack to the developer for manual manipulation.

Checklist:

@sambacha sambacha requested a review from a team as a code owner July 9, 2023 17:54
@Alhadis Alhadis changed the title feat(language): ♞ Huff Add support for Huff Jul 10, 2023
@sambacha
Copy link
Author

Unsure of why 3.2 test failing

@lildude
Copy link
Member

lildude commented Jul 11, 2023

Unsure of why 3.2 test failing

Capitalisation

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.

You can fix the tests by capitalising the h in the samples directory name: samples/huff/ -> samples/Huff/

lib/linguist/languages.yml Outdated Show resolved Hide resolved
sambacha added a commit to manifoldfinance/github-linguist that referenced this pull request Jul 12, 2023
@sambacha
Copy link
Author

You can fix the tests by capitalising the h in the samples directory name: samples/huff/ -> samples/Huff/

Subtle overlook on my part, apologies and thank you for pointing that out!

Is their insufficient usage of this language to be considered for inclusion right now (re: label of pending popularity)?

Thanks for the feedback, appreciate it.

@sambacha sambacha requested a review from lildude July 12, 2023 05:30
@lildude
Copy link
Member

lildude commented Jul 12, 2023

Is their insufficient usage of this language to be considered for inclusion right now (re: label of pending popularity)?

Yup. Usage of all extensions being added needs to meet usage requirements so you might want to consider removing the .hf extension for now as it appears to be significantly less used than .huff from a quick look. Users can use an override once this PR is merged and deployed.

I've used the search link you've provided in the template (I'm AFK on mobile only ATM) so if you can improve that to match more files, then go for it.

@sambacha
Copy link
Author

Is their insufficient usage of this language to be considered for inclusion right now (re: label of pending popularity)?

Yup. Usage of all extensions being added needs to meet usage requirements so you might want to consider removing the .hf extension for now as it appears to be significantly less used than .huff from a quick look. Users can use an override once this PR is merged and deployed.

I've used the search link you've provided in the template (I'm AFK on mobile only ATM) so if you can improve that to match more files, then go for it.

Thank you for the clarification. One would think that file extensions would be a transitive relational property, I removed the short hand .hf per your suggestion.

Thanks for your patience, much obliged!

@sambacha
Copy link
Author

Resolved the last remaining issue, an overlook on my part, apologies!

After looking at the example files again, I have a question:

Would creating an example piece of code (in our case a contract) that is not meant to work (or be a good example). Rather it should be written for the purposes of demonstrating good syntax highlighting by the lexer, even if otherwise hazardous/not idiomatic of the language per se?

I encountered this issue while trying to fix the Nix grammar that linguist was using before tree-sitter support was made available.

Example pre tree-sitter the grammar was not able to correctly highlight this as valid nix.

Screenshot 2023-07-14 at 8 06 43 PM

@lildude
Copy link
Member

lildude commented Jul 15, 2023

Would creating an example piece of code (in our case a contract) that is not meant to work (or be a good example). Rather it should be written for the purposes of demonstrating good syntax highlighting by the lexer, even if otherwise hazardous/not idiomatic of the language per se?

Such a contrived example would only really be useful for the grammar, not linguist. We need real world samples as they're used to train the classifier, not validate syntax highlighting, so a contrived example is more likely to lead to misidentification.

@sambacha
Copy link
Author

Is their insufficient usage of this language to be considered for inclusion right now (re: label of pending popularity)?

Yup. Usage of all extensions being added needs to meet usage requirements so you might want to consider removing the .hf extension for now as it appears to be significantly less used than .huff from a quick look. Users can use an override once this PR is merged and deployed.

I've used the search link you've provided in the template (I'm AFK on mobile only ATM) so if you can improve that to match more files, then go for it.

There are now over 1.1k file (164 ms) results found using the query: https://github.com/search?q=%28path%3A*.huff+OR+path%3A*.Huff%29&type=code

Appreciate the patience @lildude - have fixed all outstanding issues, please let me know how this can move forward. Cheers!

@sambacha
Copy link
Author

Circling back around @lildude @Alhadis - all the requests per your last review have been addressed.

@lildude
Copy link
Member

lildude commented Sep 23, 2023

From a quick glance, things are looking good now though usage is still too low when we exclude forks and the huff-language user. I'll keep reviewing the usage with each release.

@lildude lildude dismissed their stale review September 23, 2023 02:46

Issues resolved

@erhant
Copy link

erhant commented Sep 24, 2023

popularity about to rise with the upcoming Huff hackathon! :)

@sambacha
Copy link
Author

From a quick glance, things are looking good now though usage is still too low when we exclude forks and the huff-language user. I'll keep reviewing the usage with each release.

Thanks for the clarification, I appreciate the follow-up. Will circle back around and check as well, we are at 932 reported findings per your query. 

Cheers, 

  • Sam

@erhant
Copy link

erhant commented Jan 12, 2024

From a quick glance, things are looking good now though usage is still too low when we exclude forks and the huff-language user. I'll keep reviewing the usage with each release.

The query above now returns over 1.2k files! Plus, Huff icon just got merged to a widely-used icon theme material-extensions/vscode-material-icon-theme#2190

@sambacha
Copy link
Author

May we get a review @lildude - we should be good to go!

Please let me know if you want any additional changes or clean up of git commits etc.

Thanks!

@lildude
Copy link
Member

lildude commented Feb 23, 2024

Reviews of all pending popularity PRs takes place when I'm preparing for the next release. This will be reviewed again then.

@sambacha
Copy link
Author

sambacha commented Mar 6, 2024

Reviews of all pending popularity PRs takes place when I'm preparing for the next release. This will be reviewed again then.

When do you plan on preparing a next release? I ask because I was going to submit a fix for the TOML grammar that is being used, which would involve replacing the current one in use as its not actively maintained.

Thanks for the follow up, as linguist is a production dependency for GitHub I appreciate the care taken!

@lildude
Copy link
Member

lildude commented Mar 6, 2024

I need to do it before my colleagues start preparing the next release of Enterprise Server which starts around the middle of the month, so it'll probably be sometime next week or early the week after.

@sambacha
Copy link
Author

I need to do it before my colleagues start preparing the next release of Enterprise Server which starts around the middle of the month, so it'll probably be sometime next week or early the week after.

Thanks for the update, will be sure to keep an eye out in case any changes/updates need to be made.

Cheers!

@sambacha
Copy link
Author

sambacha commented Apr 8, 2024

I need to do it before my colleagues start preparing the next release of Enterprise Server which starts around the middle of the month, so it'll probably be sometime next week or early the week after.

hey @lildude just circling back seeing if there is anything on my end needed to get this sorted!

Thanks again!

@lildude
Copy link
Member

lildude commented Apr 9, 2024

As previously stated, nothing more is needed on this PR and I review popularity with each release. Our requirements still weren't met when I reviewed for the last release. I do not comment each time as it's too time consuming, so no news is good news. There's also no need to keep merging in master unless the UI indicates there is a conflict.

@sambacha
Copy link
Author

sambacha commented Jun 1, 2024

As previously stated, nothing more is needed on this PR and I review popularity with each release. Our requirements still weren't met when I reviewed for the last release. I do not comment each time as it's too time consuming, so no news is good news. There's also no need to keep merging in master unless the UI indicates there is a conflict.

Thanks, I only ask because I see a release was made on May 2nd, so just making sure we do not get left behind!

@sambacha
Copy link
Author

As previously stated, nothing more is needed on this PR and I review popularity with each release. Our requirements still weren't met when I reviewed for the last release. I do not comment each time as it's too time consuming, so no news is good news. There's also no need to keep merging in master unless the UI indicates there is a conflict.

Just circling back on this PR, as was updating #6988 for adding RFC 5545/iCAL support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants