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

go: replace deprecated oracle with guru #6774

Closed
wants to merge 1 commit into from
Closed

Conversation

edrex
Copy link
Contributor

@edrex edrex commented Aug 5, 2016

updates go-oracle -> go-guru. It's the same command, probably renamed because of recent legal disputes..

Outstanding issues with this patch:

  • change prefix for guru commands from mr to mf to separate from rename command
  • since rename prefix now only has a single item, surface that item as mr
  • Reuse go-guru-map rather than maintaining a separate list of bindings.
  • Calling go-guru-definition results in an error:
    cdr: Wrong type argument: listp, "definition"
    This might be due to upstream, further investigation is needed.
  • On OSX, GOPATH needs to be set via launchd or guru initialization is skipped, see http://stackoverflow.com/questions/135688/setting-environment-variables-in-os-x. Document this.
  • Can we do something smarter about providing an entry point package for some guru subcommands (callers, ...)? Maybe we can figure out the package name at the top level of the project and provide that as a default value in the prompt? What about persisting it across emacs sessions per-project?

Fixes #6772.

change to prefix for guru commands from `mr` to `mf`
to separate from rename commands. Better prefix?

Fixes syl20bnr#6772.
@TheBB
Copy link
Collaborator

TheBB commented Aug 6, 2016

@bogdanteleaga does this look ok to you?

@TheBB
Copy link
Collaborator

TheBB commented Aug 6, 2016

Maybe you can have a look at the conventions document to make sure that the bindings make sense. SPC m r is for refactoring and there is no SPC m f. Some of these bindings look like they should be on SPC m g (like jump to definition).


(defun go/init-go-rename()
(use-package go-rename
:init
(spacemacs/declare-prefix-for-mode 'go-mode "mr" "rename")
Copy link
Contributor

Choose a reason for hiding this comment

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

Except you nuked all keybindings for this prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh you're right. I thought there were some "rename" bindings. what does the go-rename package do?

Copy link
Contributor

@robbyoconnor robbyoconnor Aug 7, 2016

Choose a reason for hiding this comment

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

How did you miss the fact that no keybindings with that prefix existed? I don't know -- I don't do too much go coding.

Copy link
Contributor Author

@edrex edrex Aug 8, 2016

Choose a reason for hiding this comment

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

I just quickly fixed this because I became blocked while doing some go work by an issue with oracle.

It looks like the main entry point for go rename is actually mapped to mrr (see the next line).

I can just put the bindings back under mr if folks are happy with them.

Copy link
Contributor

Choose a reason for hiding this comment

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

hrm...I think I missed that.

Choose a reason for hiding this comment

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

any update?

@edrex
Copy link
Contributor Author

edrex commented Aug 11, 2016

Anything I can do to move this forward? I can see the key remapping being possibly controversial; I'm happy to change it back.

Potential go-coding reviewers? @bogdanteleaga

Thanks all

@bogdanteleaga
Copy link
Contributor

Hey, sorry, I saw the email initially and completely forgot about it.

Thanks for the contribution, I was not aware they were going to rename oracle, I could never use it that much because it didn't work properly for the projects I was working on. That being said, I always found the bindings being under SPC m r to be rather weird.

As for the point @TheBB made, some of them look like they could work as a replacement for go to defintion (go-guru-definition) or documentation (go-guru-describe). But I believe we should just let the whole thing sit under its own keybinding, mostly because guru/oracle doesn't work without setting a scope and that might be confusing to new users. Most of the functionality it provides is rather unique and doesn't really fit under the rest of the conventions we use in spacemacs as far as I am aware.

What I'd like is at least some warning sign(probably in the documentation), that oracle is deprecated and we should use guru now. Even though oracle users are most likely aware, I don't think it would hurt to have it around for a while.

tl;dr 👍, but add an explanation in the docs.

@edrex
Copy link
Contributor Author

edrex commented Aug 11, 2016

Ok, I'll update the docs warning users about oracle deprecation. Thanks for taking a look!

@TheBB
Copy link
Collaborator

TheBB commented Aug 17, 2016

Ping! I was about to merge this but looks like it's not updated yet.

@edrex
Copy link
Contributor Author

edrex commented Aug 17, 2016

@TheBB yeah I still need to do another pass, will update when ready

@bmag bmag mentioned this pull request Aug 27, 2016
@ghost
Copy link

ghost commented Aug 28, 2016

Is it possible to reuse the go-guru-map instead of maintain another set in the go/init-go-guru?

@edrex
Copy link
Contributor Author

edrex commented Aug 29, 2016

@enzochiau I'd like that. Example needed. Updated the issue text to add that as a todo.

go-path "/src/golang.org/x/tools/cmd/oracle/oracle.el")
(spacemacs/declare-prefix-for-mode 'go-mode "mr" "rename")
go-path "/src/golang.org/x/tools/cmd/guru/go-guru.el")
(spacemacs/declare-prefix-for-mode 'go-mode "mf" "guru")

Choose a reason for hiding this comment

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

why use mf? what about mj? ; j for jump(definition)

@jredville
Copy link
Contributor

I'm new to emacs, but I use go and go-guru and want to get involved. Is there anything I can do to help out here?

@TheBB
Copy link
Collaborator

TheBB commented Sep 29, 2016

I was waiting for this.

Ok, I'll update the docs warning users about oracle deprecation.

If the PR works as is and someone submits a new one with documentation update on top, I can merge.

@jredville
Copy link
Contributor

I'll take a look this weekend

@jredville
Copy link
Contributor

@TheBB new one submitted building on this one

@JAremko
Copy link
Collaborator

JAremko commented Oct 15, 2016

@TheBB It seems that golang.org/x/tools/cmd/oracle is gone now. So this PR should have high priority.

@TheBB
Copy link
Collaborator

TheBB commented Oct 17, 2016

Thanks! Cherry-picked in develop as part of #7242. You can safely delete your branch..

@TheBB TheBB closed this Oct 17, 2016
@edrex edrex deleted the go-guru branch October 17, 2016 15:57
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

9 participants