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 protobuf support #6748

Merged
merged 1 commit into from
Jan 30, 2024
Merged

Conversation

MordFustang21
Copy link
Contributor

@MordFustang21 MordFustang21 commented Jan 26, 2024

Release Notes:

  • Added protobuf syntax highlighting (#5160).

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Jan 26, 2024
@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Jan 26, 2024

Looks much better now, thank you 🚀

image

I think we would need to add a bit more:

  • optional keyword highlights
  • oneof variant highlighting also asks for more colors
  • outline.scm so a person could navigate over the next with cmd-shift-o menu + enable the gutter showing regions on top
  • ? maybe, add some slight color change to the field names, so they do not mix with the unknown identifiers
  • and a small formatting nit the CI complained about needs to be fixed

Cargo.toml Outdated Show resolved Hide resolved
crates/zed/Cargo.toml Outdated Show resolved Hide resolved
@maxdeviant maxdeviant changed the title languages: add protobuf support Add protobuf support Jan 26, 2024
Copy link
Contributor

@mikayla-maki mikayla-maki left a comment

Choose a reason for hiding this comment

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

Thank you so much for adding this language! I've been meaning to do this for ages 😅.

I added a few small things, and agree with my colleagues about the alphabetizing, highlighting, and outline file. If you'd like to drop by our channels or fireside hacks I'd be happy to help build the outline file. It would be great to get this one merged in :D

crates/zed/src/languages/proto/folds.scm Outdated Show resolved Hide resolved
crates/zed/src/languages/proto/highlights.scm Outdated Show resolved Hide resolved
@mikayla-maki mikayla-maki marked this pull request as draft January 26, 2024 19:49
@MordFustang21 MordFustang21 force-pushed the zed-protobuf branch 3 times, most recently from e5666b0 to 4bad7b0 Compare January 27, 2024 19:36
@MordFustang21
Copy link
Contributor Author

I've addressed most of the above comments. There are still some outstanding issues though that I'm not quite sure how to tackle.

  1. oneof variant highlighting also asks for more colors
  2. ? maybe, add some slight color change to the field names, so they do not mix with the unknown identifiers

@mikayla-maki I added an outline file with some queries that felt most relevant. Happy to pair on it sometime if you feel there's more to add.

@MordFustang21 MordFustang21 force-pushed the zed-protobuf branch 2 times, most recently from 108430a to ca349a3 Compare January 28, 2024 00:11
@MordFustang21 MordFustang21 marked this pull request as ready for review January 28, 2024 17:15
@mikayla-maki
Copy link
Contributor

Thank you for this PR! :D

@SomeoneToIgnore
Copy link
Contributor

Let's add a new doc entry into https://github.com/zed-industries/zed/tree/main/docs/src/languages list and merge this 🎉

@MordFustang21
Copy link
Contributor Author

Added doc file

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

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

I think it's still a bit odd to have the fields as defaults, but since I have no good proposal on how to capture it, let's move on.

Thank you for taking a stab at this, now Zed repo is better highlighted.

@SomeoneToIgnore SomeoneToIgnore merged commit 7bfa584 into zed-industries:main Jan 30, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants