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

A way to customize what minor modes jedi:doc can use #29

Merged
merged 3 commits into from Jan 31, 2013

Conversation

Projects
None yet
2 participants
@jkpl
Contributor

jkpl commented Jan 31, 2013

I set the default jedi:doc minor mode to view-mode, which makes the doc buffers read only.

@tkf

This comment has been minimized.

Owner

tkf commented Jan 31, 2013

Hi, thanks for the pull request. However, I am not sure about this change. Because you can already do this:

(setq jedi:doc-mode
      (lambda ()
        (rst-mode)
        (view-mode)))

What do you think? What jedi:doc-minor-modes adds to it?

But I like the idea of calling view-mode by default.

@jkpl

This comment has been minimized.

Contributor

jkpl commented Jan 31, 2013

It adds convenience. I didn't know you could set minor modes that way until I looked at the source code. Also, the doc for the customization states that it used for setting the major mode. Circumventing the limitation with a lambda function seems kinda "hacky" solution to me especially since it is so easy to add the same functionality to the actual code. :-)

Since jedi:doc-mode is just a symbol that is called, I guess it could be combined with the new customization variable into one variable which has a list of all the modes. jedi:show-doc then would call all those functions in the list.

Another alternative would be to make a hook variable for the jedi doc view, and run all the functions (modes and everything else) attached to that hook when the doc buffer is opened.

@tkf

This comment has been minimized.

Owner

tkf commented Jan 31, 2013

You are right about that jedi:doc-mode does not sound like you can set any callable to it. So I thought I'd change it to jedi:doc-hook so that it is very clear that you can set any callable to it. However, you want to make sure that major mode called first because otherwise it reset minor modes. So, actually having two overlapping customizable variables makes sense. I was almost sure I was going to reject this PR but turned out I was wrong :)

Can you rename jedi:doc-minor-modes to jedi:doc-hook and use run-hooks to call it?

@jkpl

This comment has been minimized.

Contributor

jkpl commented Jan 31, 2013

As far as I know, hooks are typically left empty by default, so I did so too. If you'd like to have view-mode on by default, I can add back the default value to the hook or make jedi:show-doc call view-mode.

@tkf

This comment has been minimized.

Owner

tkf commented Jan 31, 2013

I think it's good to have view-mode as default. It's better than hard coding it because you can turn off if you want. So, can you add it if you don't oppose to it?

@jkpl

This comment has been minimized.

Contributor

jkpl commented Jan 31, 2013

I don't mind. I'm gonna use it anyway. :)

tkf added a commit that referenced this pull request Jan 31, 2013

Merge pull request #29 from jkpl/master
Add new configurable: jedi:doc-hook

@tkf tkf merged commit 3640b65 into tkf:master Jan 31, 2013

1 check passed

default The Travis build passed
Details
@tkf

This comment has been minimized.

Owner

tkf commented Jan 31, 2013

Thanks!

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