hvesalai / emacs-scala-mode

The definitive scala-mode for emacs
http://ensime.org
GNU General Public License v3.0
361 stars 68 forks source link

Merge ob-scala into scala-mode #107

Closed reactormonk closed 8 years ago

reactormonk commented 8 years ago

Needs some adjustments.

CLAassistant commented 8 years ago

CLA assistant check
All committers have signed the CLA.

fommil commented 8 years ago

sorry, I wasn't watching the repo and only seen this now.

fommil commented 8 years ago

can you think of a test for this? @hvesalai are you ok with this? the dependency on org-mode is optional and won't break things for users who don't use it.

reactormonk commented 8 years ago

https://github.com/nim-lang/nim-mode/blob/master/tests/test-ob-nim.el might be a way.

fommil commented 8 years ago

that sounds good! Even better if it can be written to use ert (easier to add to CI, which I'm aware we don't have yet).

reactormonk commented 8 years ago

Any tests yet? I don't see any in the emacs-scala-mode repo. Buttercup works just fine in CI. https://travis-ci.org/nim-lang/nim-mode

fommil commented 8 years ago

I'll set up drone when we have tests to run, like ensime.

fommil commented 8 years ago

if you rebase you'll find a basic CI setup to add some tests. You might need to add a command to .drone.yml to run them, e.g. the ert-runner

fommil commented 8 years ago

@reactormonk the package has been renamed to scala-mode so your file will need to change.

fommil commented 8 years ago

I suppose you don't need to change the file name because its ob-scala by convention.

fommil commented 8 years ago

I'll merge now, you can add tests later :smile:

Can you please send a PR to melpa to remove this as a separate package?

reactormonk commented 8 years ago

https://github.com/melpa/melpa/pull/3908

fommil commented 8 years ago

cool, it will be released with the next MELPA sweep.

tarsius commented 8 years ago

Org also comes with an ob-scala implementation, see http://repo.or.cz/org-mode.git/blob/ddd58ff99a8684f6bcae24ffa5050857f533e26d:/lisp/ob-scala.el (from the maint branch).

You might want to merge the two implementations and put the result inside only one of the repositories.

fommil commented 8 years ago

@reactormonk can you please look into this? Who wrote the one in org?

reactormonk commented 8 years ago

@fommil How would I specify which ensime to use? There's a session argument to org-mode source blocks, so you could specify which ensime to connect to for evaling. The one in org-mode just does plain scala without libs.

fommil commented 8 years ago

https://github.com/ensime/emacs-scala-mode/blob/master/ob-scala.el is independent of ensime.

I don't like having two implementations of the same thing, can you please find out who the author of the org-mode code is and coordinate with them to merge the implementations? It might make sense for this functionality to live in the org codebase if that is the case.

fommil commented 8 years ago

oh, you do depend on ensime in ob-scala. I didn't pick up on that before. That's not good.

I think it would be best if this code moved into org.

fommil commented 8 years ago
git l lisp/ob-scala.el
*   6345de2 : Kyle Meyer, 5 months ago : Merge branch 'maint'
|\  
| * 6bc48c5 : Kyle Meyer, 5 months ago : Update copyright years
* | f0bf77e : Nicolas Goaziou, 7 months ago : Activate lexical binding in some libraries
|/  
* 0839dc4 : Kyle Meyer, 8 months ago : Protect remaining apostrophes in docstrings
* ecf3a4a : Paul Eggert, 1 year, 5 months ago : Backport remaining changes from commit 7e09ef0
* d067982 : Aaron Ecay, 1 year, 4 months ago : babel: Remove functions which are just indirection around org-babel-script-escape
* 7d9a883 : Bastien Guerry, 2 years, 5 months ago : Update copyright years again.
* 0beda99 : Bastien Guerry, 2 years, 5 months ago : Revert "Update copyright years."
* 2110559 : Bastien Guerry, 2 years, 5 months ago : Update copyright years.
* f95641c : Bastien Guerry, 2 years, 7 months ago : Backport changes from Emacs revs 115081 and 115082
*   679dce0 : Bastien Guerry, 3 years, 5 months ago : Merge branch 'maint'
|\  
| * 31c1aea : Bastien Guerry, 3 years, 5 months ago : Various small fixes
* |   5fc740a : Bastien Guerry, 3 years, 5 months ago : Merge branch 'maint'
|\ \  
| |/  
| * 72bc144 : Bastien Guerry, 3 years, 5 months ago : Update Copyright lines to match Emacs format.
* |   60b23bd : Bastien Guerry, 3 years, 5 months ago : Merge branch 'maint'
|\ \  
| |/  
| * 98cd468 : Bastien Guerry, 3 years, 5 months ago : Update copyright years.
* | ff00818 : Eric Schulte, 3 years, 6 months ago : requiring ob now pulls in all of Babel
* | 78cdf14 : Eric Schulte, 3 years, 6 months ago : org-babel-result-cond - unified handling of results
|/  
* de7766f : Caio Tiago Oliveira, 3 years, 8 months ago : Babel: add results value support to Scala
* 5fc07bc : Bastien Guerry, 3 years, 8 months ago : Declare functions and variables.
* 222eae4 : Bastien Guerry, 3 years, 8 months ago : Fix error messages: don't use a dot at the end
* 70dd119 : Bastien Guerry, 3 years, 10 months ago : Massive code clean-up.
* 5061de6 : Eric Schulte, 4 years, 3 months ago : integrating ob-scala and ob-io into Org-mode, with code cleanup
* 806ce38 : Andrzej Lichnerowicz, 4 years, 3 months ago : support for execution of Scala code blocks

and the vast majority of the file is by Andrzej Lichnerowicz @unjello is that you?

reactormonk commented 8 years ago

Ensime is basically required for a) executing things without too much of a startup time b) adding in libraries. I could however make it optional only if you specify the corresponding ensime buffer via the session argument.

fommil commented 8 years ago

it's a bit weird to have a cyclic dependency. ensime depends on scala-mode which depends on ensime. Can you please PR to remove this and add to ensime-emacs in the meantime?

There is precedent in the ob-scala.el file to depend on external dependencies, so I recommend sending a PR to them to update their support with your enhancements so that there is no namespace and language collisions. It would also be good if you could check that your code was compatible with theirs, for now.

fommil commented 8 years ago

I guess it's not a PR to these old school emacs packages, probably a letter signed in triplicate and delivered to a post office box.

tarsius commented 8 years ago

I guess it's not a PR to these old school emacs packages, probably a letter signed in triplicate and delivered to a post office box.

The process is described here: https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html.

tarsius commented 8 years ago

Also see https://www.emacswiki.org/emacs/Copyright_Assignment.

unjello commented 8 years ago

@fommil Hey! Sorry for the delay, I have totally missed that in my inbox... yes, that is me.