parsonsmatt / intero-neovim

A neovim plugin for Intero, forked from ghcmod-vim
218 stars 28 forks source link

Implement Python functions natively in VimL #91

Closed jez closed 7 years ago

jez commented 7 years ago

We use Python in exactly two places. In both places, the same thing can be done in VimL as easily or more easily than in Python.

We have had numerous issues in the past specifically related to our use of Python. People use different versions of Python. Getting Python to work with Vim requires jumping through a few hoops.

All of this extra abstraction is unnecessary. I've replaced the two small Python functions with two small VimL functions, and removed all the Python dependency-management config.

Closes #89. Closes #90.

jez commented 7 years ago

To reviewers: I've broken this PR up into two commits.

The first commit is much smaller than the second. The second commit is entirely deletions.

parsonsmatt commented 7 years ago

I'm generally in favor of anything that removes 9x as much code as it adds :smile:

decentral1se commented 7 years ago

LGTM too! I also started to think it was a bit much. Will also solve https://github.com/parsonsmatt/intero-neovim/issues/83?

Maybe we can think about using vader for testing now.

rdnetto commented 7 years ago

That looks a lot more readable than I would have originally thought - nice work! :)

One thing though - could you preserve the unit tests for stripping the control characters? That'd do a lot to increase confidence that the new and old implementations work the same way. A quick google indicates Vader is popular for unit testing Vimscript, though I don't have any opinions on which framework would be best for this.

jez commented 7 years ago

I set up the testing infrastructure using Vader (because you two suggested it, and I'd used it before).

Apart from the mess of actually configuring everything to play well on Travis, using Vader is pretty seamless. We can definitely start requiring tests on new PRs going forward especially for unit-testable functions.

I addressed all the other feedback; @rdnetto please take a look.

jez commented 7 years ago

I'm generally in favor of anything that removes 9x as much code as it adds 😄

Unfortunately, adding in the testing code jumps it back up to only 3x as much code removed as added 😝

decentral1se commented 7 years ago

yiipppeeeeee

jez commented 7 years ago

@rdnetto @parsonsmatt I looked into getting Travis to use nvim instead of vim for running the Vader tests. Here's what's up

So our options are:

  1. Use vim to run the tests, instead of nvim.
    • pros: Involves the least amount of work
    • cons: We'd only be able to write tests for the version-agnostic parts of our code
  2. Use the unstable PPA to run our tests.
    • pros: Happy medium between "do little work" and "still use Neovim"
    • cons: Our tests might break due to random nightly build idiosyncrasies.
  3. Update Travis to 16.04, and use the stable PPA.
    • pros: Uses Neovim, while not having to worry about nightly builds.
    • cons: After 15 minutes of Googling I'm still not sure how to do this.

I'm leaning towards (1) or (2). I could look into (3) if one of you really think it's worth it. It's also fine if someone wants to help me with (3) by pushing to this branch.

parsonsmatt commented 7 years ago

Generally speaking, we're going from "no vimscript tests" to "some vimscript tests," so all three of those options are improvements over the status quo.

I'm not a fan of 1. I've chosen to specifically only support Neovim because I only use neovim, and vim didn't have the required features at the time of initially writing the plugin. Porting to vim isn't valuable work IMO. That said, it is an improvement over what we have now, and I'm fine with lazily evaluating the testing situation :)

2) seems like a fine solution. If tests are randomly broken due to an unstable release, we can run the tests manually/locally before merging.

3) seems like we'd have to switch to the Docker based infrastructure, write a Dockerfile, etc. to get the business we want. That's not all that much work, but it does require switching to the slower container-based builds.

jez commented 7 years ago

@parsonsmatt @rdnetto I've implemented option #2. Please take another look.