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 recipe for ob-elm #6605

Merged
merged 1 commit into from
Jan 4, 2020
Merged

Add recipe for ob-elm #6605

merged 1 commit into from
Jan 4, 2020

Conversation

BonfaceKilz
Copy link
Contributor

Brief summary of what the package does

Org-Babel support for evaluating Elm code

Direct link to the package repository

https://github.com/BonfaceKilz/ob-elm

Your association with the package

I'm the author

Relevant communications with the upstream package maintainer

None needed

Checklist

Please confirm with x:

  • The package is released under a GPL-Compatible Free Software License.
  • I've read CONTRIBUTING.org
  • I've used the latest version of package-lint to check for packaging issues, and addressed its feedback
  • My elisp byte-compiles cleanly
  • M-x checkdoc is happy with my docstrings
  • I've built and installed the package using the instructions in CONTRIBUTING.org
  • I have confirmed some of these without doing them

Note:

  • When using package-lint, I found:
6 issues found:

6:3: warning: You should include standard keywords: see the variable `finder-known-keywords'.
51:0: error: "org-babel-elm-eoe" doesn't start with package's prefix "ob-elm".
105:0: error: "org-babel-elm-initiate-session" doesn't start with package's prefix "ob-elm".
112:0: error: "org-babel-load-session:elm" doesn't start with package's prefix "ob-elm".
112:0: error: `org-babel-load-session:elm' contains a non-standard separator `:', use hyphens instead (see Elisp Coding Conventions).
134:0: error: "org-babel-elm-var-to-elm" doesn't start with package's prefix "ob-elm".

I ignored the above errors because fixing them would mean going against conventions set here in the officicial org template for extending languages

@riscy
Copy link
Member

riscy commented Dec 23, 2019

Thanks, looks useful.

ob-elm.el

M-x checkdoc (using version 0.6.1):
No issues! 💯

M-x package-lint-current-buffer (using version 20191124.132):

6 issues found:

6:3: warning: You should include standard keywords: see the variable `finder-known-keywords'.
<snip>

The org-babel conventions seem fine, but the above one could be addressed.

M-x byte-compile-file (using Emacs 26.3):
Per CONTRIBUTING.org, consider using lexical binding
(the following is with lexical binding enabled)

ob-elm.el:64:29:Warning: `assq' called with 8 args, but requires 2
ob-elm.el:81:36:Warning: assq called with 8 arguments, but accepts only 2
ob-elm.el:64:47:Warning: reference to free variable `pThe'
ob-elm.el:64:52:Warning: reference to free variable `header-args'
ob-elm.el:64:64:Warning: reference to free variable `passed'
ob-elm.el:64:71:Warning: reference to free variable `to'
ob-elm.el:64:74:Warning: reference to free variable `the'
ob-elm.el:64:78:Warning: reference to free variable `src'
ob-elm.el:64:82:Warning: reference to free variable `block\.arams'

In end of data:
ob-elm.el:153:1:Warning: the following functions are not known to be defined:
    org-trim, org-strip-quotes

I think a typo snuck in for the first chunk. A (require 'org) is needed for org-trim, and I think org-strip-quotes isn't available with Emacs 26.3's version of org (I don't know about Emacs 27), so you should add a later-versioned Package-Require on org. Or you could use org-babel-strip-quotes for backward compatibility.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Dec 23, 2019
@BonfaceKilz
Copy link
Contributor Author

BonfaceKilz commented Dec 24, 2019

@riscy Done. I've opted to specify the org version. Here's the merged(upstream) PR: https://github.com/BonfaceKilz/ob-elm/pull/9/files

Here are the specific commits that address each of the points addressed above:

@riscy riscy removed the awaiting-upstream Awaiting action from an upstream maintainer label Dec 24, 2019
@riscy
Copy link
Member

riscy commented Jan 2, 2020

This is almost there, but I'm having trouble with this expression here (maybe you can clarify):

  (add-hook 'elm-mode-hook
            (lambda ()
              (setq-local comint-prompt-regexp
                          (concat elm-prompt-regexp "\\|^λ?> "))))

Does this have any effect? For instance I don't think elm-mode pays any attention to comint-prompt-regexp (but perhaps elm-repl does? Do we care it does for the purposes of this package?)

For reference, I see ob-haskell has something similar,

(add-hook 'inferior-haskell-hook
            (lambda ()
              (setq-local comint-prompt-regexp
                          (concat haskell-prompt-regexp "\\|^λ?> "))))

but the hook here is being added to inferior-haskell (i.e. the REPL) instead of haskell-mode.

Since I'm not too familiar with ob stuff, I'm not sure what the goal of changing the prompt is. But usually a change like that (one that a user might want to undo) should be done through a minor mode, not as a hook that gets snuck in when a function is called. In addition, anything you add as a hook in a package should probably be a named function, not an anonymous one -- named functions can be easily redefined, and named functions are easier to remove from hooks once they're in there.

That was a bit long-winded, but if that expression isn't having any effect, I wonder if you could just remove it?

@BonfaceKilz
Copy link
Contributor Author

Thanks for the review. I've learnt something about things to watch out for when creating a package 😄 👍

I'm not an expert on ob myself... I really wanted to evaluate elm inside org so I hacked up something that works. With regards, to the minor hook, you are right... It has no effect on the elm buffer(and anyways I don't think it matters because the output in the org buffer is what matters). Anyways, I've removed it here

@riscy
Copy link
Member

riscy commented Jan 4, 2020

Great, thanks for this. And, welcome to MELPA!

@riscy riscy merged commit 9109c17 into melpa:master Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants