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

haskell: Add all haskell-mode keybindings to haskell-literate-mode #3459

Conversation

codygman
Copy link

This actually isn't working. For some reason haskell/init-literate-haskell-mode isn't being called and I'm not sure why. The code is a simple copy of init-haskell-mode with small modifications for haskell-literate-mode.

Issue #3458 is related to this.

@d12frosted
Copy link
Collaborator

This is because layer-name/init-package-name is called for package-name in layer-name. Haskell literate mode is part of haskell-mode pckage. That's why all literate haskell key bindings should be configured in haskell/init-haskell-mode function.

@d12frosted
Copy link
Collaborator

What you might want to do is to copy some key bindings from haskell-mode to literate mode. Check the key bindings in haskell-mode.

@codygman
Copy link
Author

@d12frosted Alright, that's what I did originally but figured it would be better to separate into a new function. Should have known that was wrong when it didn't work.

@d12frosted
Copy link
Collaborator

By the way, here is some useful info about creating new layer in spacemacs. Not that you are creating one, but worth reading when you supporting / improving existing layers 😄

Alright, that's what I did originally but figured it would be better to separate into a new function. Should have known that was wrong when it didn't work.

Yeah, it's a good idea to separate into different functions, but you copied the whole thing 😄

P. S. After you work on this PR is done (it's working and you thin it's ready to be merged / cherry picked) you should squash all commits into one, since it's simple improvement 😄

@codygman codygman force-pushed the add-keybindings-haskell-literate-mode branch from 989741f to 258b56a Compare October 19, 2015 05:38
@codygman
Copy link
Author

@d12frosted Thanks for the help. I think I should add the literate-haskell keybindings to my own layer until they get into master. Is that how you recommend handling stuff like that?

@codygman
Copy link
Author

@d12frosted Thanks again! I made a temporary layer to add those keybindings until these changes are merged with master.

@TheBB
Copy link
Collaborator

TheBB commented Oct 19, 2015

Thanks for this, but can you do something about the code duplication?

(dolist (m '(haskell-mode literate-haskell-mode))
  (evil-leader/set-key-for-mode m
    ;; ....
    ))

@codygman
Copy link
Author

Sure! I had a feeling something like that was possible.
On Oct 19, 2015 7:43 AM, "Eivind Fonn" notifications@github.com wrote:

Thanks for this, but can you do something about the code duplication?

(dolist (m '(haskell-mode literate-haskell-mode))
(evil-leader/set-key-for-mode m
;; ....
))


Reply to this email directly or view it on GitHub
#3459 (comment).

@codygman
Copy link
Author

Oops... pushed on accident. Have a small error and fixing it now.

@codygman codygman force-pushed the add-keybindings-haskell-literate-mode branch from 2639b95 to 17bf582 Compare October 22, 2015 05:15
@StreakyCobra
Copy link
Contributor

Another improvement may be to extract '(haskell-mode literate-haskell-mode) to a variable and use this variable in the dolist

@d12frosted
Copy link
Collaborator

@codygman @StreakyCobra @TheBB any update on this pr?

@robbyoconnor
Copy link
Contributor

You're going to need to rebase this @codygman ;) Also squash your commits.

@StreakyCobra
Copy link
Contributor

When this will have been rebased/squashed and remarks would have been addressed, I think this one is a good candidate for merge :-)

@jb55
Copy link
Contributor

jb55 commented Feb 7, 2016

I have a rebased version here: #4992

@syl20bnr
Copy link
Owner

@codygman Thank you for the PR and sorry for the late merge, I close this since it is superseded by #4992

@syl20bnr syl20bnr closed this Feb 17, 2016
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.

7 participants