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 layer #8234

Closed
wants to merge 3 commits into from
Closed

Add protobuf layer #8234

wants to merge 3 commits into from

Conversation

edrex
Copy link
Contributor

@edrex edrex commented Jan 22, 2017

Fixes #6973. Based on #7027, but updated to omit the defunct flycheck-protobuf layer.

@amol-mandhane @zhexuany @d12frosted please take a look. Let's get this merged already!

Amol Mandhane and others added 2 commits January 22, 2017 12:53
@edrex
Copy link
Contributor Author

edrex commented Jan 23, 2017

This still doesn't enable flycheck-mode automatically for protobuf-mode buffers, for me at least. I'm not sure if that's an issue in the flycheck package or this change.

@zhexuany
Copy link
Contributor

Hi,
flycheck-protobuf has a prerequisite:

you have to install protoc first. I think this is a good start point to investigate this.

@edrex
Copy link
Contributor Author

edrex commented Jan 23, 2017

I've verified that flycheck is enabled automatically for protobuf buffers in stock emacs:

SPC q D
flycheck,protobuf-mode
M-: global-flycheck-mode <Enter>
<open a protobuf file and note that flycheck-mode is enabled for that buffer>

So I'm not sure why it's not being enabled in Spacemacs. Presence or absence of

(defun protobuf/post-init-flycheck ()
  (spacemacs/add-flycheck-hook 'protobuf-mode))

seems to make no difference.

@edrex
Copy link
Contributor Author

edrex commented Jan 23, 2017

@zhexuany thanks for replying. I can enable flycheck-mode manually for the buffer (eg using SPC t s) so I don't think the prerequisite is the issue. Can you verify that it's not being enabled automatically with this patch in your setup?

This change updates the `packages.el` file to support automatically enabling
flycheck when opening a protobuf file. This is accomplished by adding `flycheck`
to the list of packages, as well as defining a `post-init-flycheck()` function.

I've tested that this works with spacemacs `0.200.7@25.1.1`.
@theckman
Copy link
Contributor

theckman commented Mar 4, 2017

@edrex I managed to get it to automatically load. But GitHub isn't letting me issue a pull request against your branch. 😒

LMK if this works for you: theckman@a4c5d95

@bmag bmag added the New Layer label Mar 4, 2017
@edrex
Copy link
Contributor Author

edrex commented Mar 5, 2017

@theckman with your change it's automatically enabling the minor mode for me too. It seems that I was missing flycheck from protobuf-packages. I reset my branch to match yours. Thanks!

This is ready for review.

@theckman
Copy link
Contributor

theckman commented Mar 5, 2017

@edrex I'm fairly ignorant to getting layers and things working with Spacemacs (and emacs in general), so I apologize if I'm missing something that should be very obvious. The one thing that seems to not work, and I'm not sure if we have the ability to make it work, is automatic completion around things with braces.

For example, if I'm writing Go which has a similar syntax to Proto3, the } in this snippet would get auto-added when I type {:

type Test struct {}

However, in the protobuf-mode files when I type { I don't get the same completion:

message Request {

I'm not sure if it's even possible to do, but I thought I'd call it out. I think in its current state, even without completion, it's much better than editing protobuf files without this layer. 👍

@edrex
Copy link
Contributor Author

edrex commented Mar 5, 2017

me too 😄

The brace matching would need to be added to the upstream mode I think:
https://github.com/google/protobuf/blob/master/editors/protobuf-mode.el

Doesn't seem like a blocker for getting this in, right?

@theckman
Copy link
Contributor

theckman commented Mar 5, 2017

Yeah, I was just starting to look there to see if maybe that was the case. Bit of a yak shave. 😛

Nope! I do not think that should be considered a blocker at all.

Copy link
Contributor

@theckman theckman left a comment

Choose a reason for hiding this comment

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

This has been tested on my personal development environment. When opening .proto files, the protobuf-mode is enabled. Also, flycheck-mode is enabled as well.

👍 on this becoming an official Spacemacs layer

@wanchaol
Copy link

wanchaol commented Mar 7, 2017

Thanks for the effort! When will this get merged?

@rargulati
Copy link

I've been testing this on my local development environment for a few weeks now and it works great! Anything I can do here to help move this along?

@edrex
Copy link
Contributor Author

edrex commented Apr 3, 2017

I think it's ready to go and just needs a signoff. Maintainers are always swamped because of the volume of pull requests. Not sure the best way to help.

@edrex
Copy link
Contributor Author

edrex commented Apr 3, 2017

@rgrinberg
Copy link
Contributor

Only @syl20bnr merges new layers. So we should ping him.

Hopefully, it doesn't take too long to merge this. It seems like a very small start for a new layer.

@theckman
Copy link
Contributor

theckman commented Apr 4, 2017

It might be slightly inappropriate, but I tried to use Twitter to ping @syl20bnr. Based on the date of Sylvain's last tweet I'm not sure it'll get seen. But I'm hopeful that this will be a small enough PR that he won't need to spend too much personal time on reviewing it.

@theckman
Copy link
Contributor

theckman commented Apr 5, 2017

With 0.200.8 out, it sounds like @syl20bnr may have some free time to 👀 outstanding PRs:

@syl20bnr
Copy link
Owner

syl20bnr commented Apr 9, 2017

Thank you ! 👍
Cherry-picked into develop branch, you can safely delete your branch.

@syl20bnr syl20bnr closed this Apr 9, 2017
@edrex edrex deleted the protobuf branch August 26, 2017 02:34
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.

None yet

8 participants