search

vheon / JediHTTP

Simple http wrapper around jedi
Apache License 2.0
40 stars 9 forks source link

Review all JediHTTP #1

Closed vheon closed 9 years ago

vheon commented 9 years ago

Just so @Valloric @micbou @oblitum @puremourning can easily review this :)

Review on Reviewable

Valloric commented 9 years ago

Thanks so much for working on this!

I haven't yet found a block of time big enough to review this, but it's coming. :)

Review status: 0 of 20 files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

This is nice. I like its simplicity a lot.

One extra thought: I was browsing the Jedi docs and saw a warning Please, note that Jedi is not thread safe. Do we know that waitress does not use threads. I searched (http://waitress.readthedocs.org) for 'thread' and it looks like it might. Now, I know ycmd didn't do anything about this, but is it something to worry about?

Reviewed 20 of 20 files at r1. Review status: all files reviewed at latest revision, 12 unresolved discussions, all commit checks successful.

.travis.yml, line 1 [r1] (raw file): Would recommend using the new infrastructure](http://docs.travis-ci.com/user/migrating-from-legacy/) for much faster builds:

sudo: false

That will help with some pain (in a later phase?) to add OS X support for Travis. You might be able to copy the scripts I made for ycmd

.travis.yml, line 5 [r1] (raw file): ycmd supports python 2.6. Do you feel that we should support that here too?

jedihttp/handlers.py, line 32 [r1] (raw file): I'm finding it slightly hard to understand why we need both a /healthy and /ready request. They seem to perform the same job.

jedihttp/handlers.py, line 36 [r1] (raw file): Maybe call this completions to match the request?

jedihttp/handlers.py, line 38 [r1] (raw file): I suspect this should be debug level logging?

jedihttp/handlers.py, line 40 [r1] (raw file): This might be a bit far, but I'm wondering if we can be a bit more abstract about how we return the parameters. The situation here is that we're just returning a dictionary representation of the Jedi Completion which-is-a BaseDefinition object. As you know, I'm no master of snake-based languages, but a quick googling suggests there's a thing called __dict__ on an object, that would populate the dictionary with all of the properties of the Completion object. That just leaves things like docstring() and params, which perhaps we could be smart about too. I don't know really. It just means that if we bump Jedi versions, then we immediately get the new properties to work with, and if ycmd wants some new data (such as params), we don't have an upstream dependency for merging.

It might be either a) too hard, or b) not possible, or even c) just too much complexity to bother with. Just thought I'd throw it out there.

In any case, we could probably factor the generation of the response as I think all of them share the common base BaseDefinition.

jedihttp/handlers.py, line 50 [r1] (raw file): I wonder if this is needed. Bottle seems to have a catchall option, which returns all exceptions as HTTPError. How useful is the additional logging?

jedihttp/handlers.py, line 63 [r1] (raw file): In the completion() method, you played these out more neatly. I think i prefer that approach :)

jedihttp/handlers.py, line 112 [r1] (raw file): i think i would have just written this whole method as:

return jedi.Script( request_data[ 'source' ],
                    request_data[ 'line' ],
                    request_data[ 'col' ],
                    request_data[ 'path' ] )

I don't think the additional vars add much, and the try/except doesn't seem to do anything.

jedihttp/utils.py, line 10 [r1] (raw file): should we check os.path.isdir here?

README.md, line 1 [r1] (raw file): We should probably (eventually):

Maybe also add your "roadmap" from the original ycmd thread?

tox.ini, line 6 [r1] (raw file): flake8 is really useful. I'd recommend running it as part of the tests, like ycmd does.

I use flake8 with syntastic and it catches a lot of typos, errors, unused imports, etc. I'm not personally fond of the cyclomatic complexity check, but the pure syntax checks are worth it IMO.

Comments from the review on Reviewable.io

vheon commented 9 years ago

That is something to reason about it. Is it true that ycmd uses this at the moment and we didn't noticed anything strange there :S @davidhalter is there something we should be extra-careful about this?

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

.travis.yml, line 1 [r1] (raw file): When I set up travis, the new infrastructure wasn't available yet. I will take a look, thanks :+1:

.travis.yml, line 5 [r1] (raw file): I don't know why I didn't do it in the first place. Will take a look at this too.

jedihttp/handlers.py, line 32 [r1] (raw file): In fact they do the same job. I designed JediHTTP to fit into ycmd, so I looked at the only other server that we had which was Omnisharp, so I mimic the design at first. I don't know if in the future JediHTTP will take much more time to be "ready" to respond to query which would make sense to have two distinct handles.

jedihttp/handlers.py, line 40 [r1] (raw file): I thought of that, and I still am actually. What I'm worry about is the amount of data, not needed by the users of the particular handle, but I could be wrong there. Will look further into it :+1:

jedihttp/handlers.py, line 50 [r1] (raw file): I will tinker with it :) it would simplify the codebase even more.

jedihttp/handlers.py, line 63 [r1] (raw file): what do you mean?

jedihttp/utils.py, line 10 [r1] (raw file): why should we?

README.md, line 1 [r1] (raw file): after we all agree that is not a bad job, I think that I will document it; even though I don't count on anyone else to use this except us in ycmd.

tox.ini, line 6 [r1] (raw file): I'll look into that as well.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

jedihttp/handlers.py, line 63 [r1] (raw file): Sorry, i meant that in completion() method you have aligned the right hand side like:

{
     'name':        value,
     'longer name': value_2
}

It just looked neater :)

jedihttp/utils.py, line 10 [r1] (raw file): unless I'm missing something, this adds all entries in the vendor_folder to sys.path. There could be other files in there I guess? Maybe not.

README.md, line 1 [r1] (raw file): You never know. The jedi guys were talking about making a server. They might fork you :)

Comments from the review on Reviewable.io

vheon commented 9 years ago

Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

jedihttp/handlers.py, line 63 [r1] (raw file): oh ok :)

jedihttp/utils.py, line 10 [r1] (raw file): the vendor directory is the one where I put the dependency, so is managed by me. We do the same exact thing in ycmd, look at AddNearestThirdPartyFoldersToSysPath.

Comments from the review on Reviewable.io

vheon commented 9 years ago

I've made a few refactorings:

Now the code is much more readable. I'm still looking at the auto generation of the response.

Review status: 16 of 22 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

.travis.yml, line 1 [r1] (raw file): apparently I was already on the new infrastructure even if I didn't have sudo: false in my .travis.yml. I've put it in anyway :)

.travis.yml, line 5 [r1] (raw file): I've added python 2.6 to my tests.

Comments from the review on Reviewable.io

davidhalter commented 9 years ago

@vheon I don't think you should be extra careful with calling Script.*. It's just that there are still bugs within Jedi (so it might crash), but then again you can just restart the server.

micbou commented 9 years ago

Should we add AppVeyor CI? I can send a PR if you want.

Reviewed 12 of 20 files at r1, 2 of 6 files at r2, 4 of 4 files at r3. Review status: 20 of 22 files reviewed at latest revision, 16 unresolved discussions, some commit checks failed.

[jedihttp/main.py, line 20 [r4]](https://reviewable.io:443/reviews/vheon/jedihttp/1#-K0Mf-mEJwaOTxuplycp) (raw file): Two blank lines before functions.

jedihttp/handlers.py, line 50 [r1] (raw file): There are some alignment issues. I would personnally do:

return {
  'completions': [ {
    ...
  }
}

jedihttp/handlers.py, line 28 [r3] (raw file): Function names should be CamelCase. This applies to all handler function names.

jedihttp/handlers.py, line 75 [r3] (raw file): Same as above.

jedihttp/utils.py, line 1 [r3] (raw file): Missing copyright and license header.

jedihttp/utils.py, line 3 [r3] (raw file): Two blank lines before functions.

tests/handlers_test.py, line 84 [r3] (raw file): This apply to all the functions in this file. In other files, CamelCase is used for the function names but not in this file (there are lower_case_with_underscores and CamelCase names). For the tests, I suggest CamelCaseTest or TestCamelCase. In YCM, we are doing CamelCase_test but I don't like it.

tests/handlers_test.py, line 106 [r3] (raw file): This goes beyond the 80 characters limit. You can rewrite it like this:

'docstring': 'f()\n\nModule method docs\nAre '
             'dedented, like you might expect'

tests/handlers_test.py, line 179 [r3] (raw file): This also goes beyond the 80 characters limit.

tests/handlers_test.py, line 95 [r4] (raw file): Missing space before request_data to respect alignment.

tests/handlers_test.py, line 150 [r4] (raw file): Missing space before request_data.

Comments from the review on Reviewable.io

vheon commented 9 years ago

I wanted to wait until the design and the code have been defined properly. Then we can add AppVeyor CI and OSX testing.

Review status: 17 of 22 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

jedihttp/handlers.py, line 28 [r3] (raw file): I think I will go the other way around and make all snake_case

Comments from the review on Reviewable.io

Valloric commented 9 years ago

This is a lot of work, thanks for doing it!

@puremourning Holy hell, Jedi is not thread-safe? I'm extremely surprised this hasn't blow up in our face so far since everything in ycmd assumes that it is.

Since we support Windows, having AppVeyor run tests as well would be a good idea.

@vheon Is jedihttp supposed to be parsed as Python 2, 3 or "both"? AFAIR Jedi itself supports both, so it might be a good idea to support both for jedihttp as well.

Reviewed 15 of 20 files at r1, 3 of 6 files at r2, 3 of 4 files at r3, 1 of 1 files at r4. Review status: 18 of 22 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

.travis.yml, line 6 [r4] (raw file): Maybe add a comment or two about TOX_ENV? It's hard to tell at-a-glance what does this do. Maybe include a link to https://testrun.org/tox/latest/.

[jedihttp/main.py, line 2 [r4]](https://reviewable.io:443/reviews/vheon/jedihttp/1#-K0Mo68iPU60dSvZR_Ge) (raw file): Perfect license choice! :)

[jedihttp/main.py, line 32 [r4]](https://reviewable.io:443/reviews/vheon/jedihttp/1#-K0Moe9s7018ZgPP47Ke) (raw file): Host should probably be command-line arg as well. (With a sane default of course.)

jedihttp/exception_plugin.py, line 29 [r4] (raw file): I'm surprised this needs a custom plugin. Wouldn't a custom Bottle error handler be less work? See ErrorHandler in handlers.py in ycmd to see how registering such a handler works. You have access to the exception in it.

jedihttp/handlers.py, line 40 [r1] (raw file): I'd personally keep it simple for now. Go with the approach you're taking, then explore doing more esoteric things later. "Launch and iterate" etc.

jedihttp/handlers.py, line 28 [r3] (raw file): @micbou It's up to @vheon to go with whatever style suits him since this is his project that we'll just happen to be users of in ycmd.

jedihttp/handlers.py, line 16 [r4] (raw file): I know you probably went with Bottle because ycmd is using it, but if I were writing ycmd from scratch, I'd go with Flask. It's better supported, way more widely used (which means more bug reports and fixes) has tons of extra features etc. Every time when I was limited by something in Bottle I ended up finding a solution for it in Flask.

So think about that. Go with whatever works best for you though.

jedihttp/handlers.py, line 29 [r4] (raw file): Why an object? Why not just True? That's valid JSON too.

tests/fixtures/py3.py, line 1 [r4] (raw file): By the name, this seems like it should contain Python 3 code, but I don't see anything that would not make it also Python 2 code. Maybe put a construct inside that will break parsing if a Python 2 interpreter is used?

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Yup I was surprised too, but it is right there at the top of the API docs

Review status: 18 of 22 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

Comments from the review on Reviewable.io

micbou commented 9 years ago

Reviewed 4 of 4 files at r5. Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

jedihttp/handlers.py, line 28 [r3] (raw file): I said that because CamelCase was used in other places. This just needs to be consistent.

Comments from the review on Reviewable.io

vheon commented 9 years ago

@Valloric yes, JediHTTP is supposed to be run by a python2 or python3 interpreter so it can complete both languages. I'm already testing it under python2.6 python2.7 and python3.3 on travis. The plan is to add AppVeyor CI and OSX testing as well for the three version of python as well.

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

[jedihttp/main.py, line 2 [r4]](https://reviewable.io:443/reviews/vheon/jedihttp/1#-K0Mo68iPU60dSvZR_Ge) (raw file): :)

[jedihttp/main.py, line 32 [r4]](https://reviewable.io:443/reviews/vheon/jedihttp/1#-K0Moe9s7018ZgPP47Ke) (raw file): Done.

jedihttp/handlers.py, line 16 [r4] (raw file): At first I was building it with Flask (I remember you saying to use Flask in https://github.com/Valloric/YouCompleteMe/issues/1278) but it was a pain to vendor it, because it has a lot of dependency. I will take another look ;)

jedihttp/handlers.py, line 29 [r4] (raw file): Bottle can't handle return True, am I wrong?

tests/fixtures/py3.py, line 1 [r4] (raw file): It is used in a python3 test. I will try putting python3 specific stuff anyway ;)

Comments from the review on Reviewable.io

vheon commented 9 years ago

Review status: 17 of 22 files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

.travis.yml, line 6 [r4] (raw file): Done.

Comments from the review on Reviewable.io

Valloric commented 9 years ago

@vheon We might want to get HMAC auth in before we depend on this in ycmd. I don't think it's strictly necessary, but it might be, and I'd rather err on the side of more security.

Review status: 17 of 22 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

jedihttp/handlers.py, line 16 [r4] (raw file): If it's a pain to vendor, then that's a solid argument for using Bottle.

jedihttp/handlers.py, line 29 [r4] (raw file): It might not be smart enough to figure out that a Python return True should be a JSON true, but if that's the case, returning a JSON string should work.

Comments from the review on Reviewable.io

vheon commented 9 years ago

I was experimenting on using Flask instead of Bottle and as I said at first I felt it was a pain to vendor; that is because at first I tried to vendor it by adding it as a submodule (kind like how we do it for YouCompleteMe and ycmd, requests, etc etc). For Flask that is a pain because it has more than one dependency:

        'Werkzeug>=0.7',
        'Jinja2>=2.4',
        'itsdangerous>=0.21',
        'click>=2.0',

At this point though I had an idea:

I use pip install Flask -t vendor to install Flask into the vendor directory with all its dependencies and I commit that. Then I use Google AppEngine google.appengine.ext.vendor (which is similar to the AddVendorFolderToSysPath function) to load Flask. This way vendoring Flask would be a breaze but I wanted to hear some opinion from someone who has more experience than me with python. @Valloric, @micbou, @puremourning, @oblitum??

Review status: 17 of 22 files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Just sounds convoluted to me. But without a doubt I am not qualified to comment on Python or GitHub best practices. If bottle works, why fix it?

Review status: 17 of 22 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

vheon commented 9 years ago

Well is really similar to what we already do actually, but instead of bring in the framework as a submodule we clone it basically. So the complexity is not that greater than it is right now. I'm just not sure if is a "pythonic" thing to do :)

Review status: 17 of 22 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

I suspect the pythonic thing to do is expect your users to set up a virtualenv, activate it and pip install -r requirements.txt. Which I guess is what you're doing on their behalf.

The other part is that even more stuff will be bundled into the paths that Jedi will see. More deps = (arguably) more false positives?

vheon commented 9 years ago

Well yes, more or less.

For the path of the Jedi process we will not have to worry about that, since Jedi will take the path to consider after https://github.com/davidhalter/jedi/issues/385 will be merged.

Review status: 17 of 22 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

micbou commented 9 years ago

The "pythonic" way would be to use setuptools, build the project as a package and upload it to PyPI. See this tutorial. Then we could do

pip install JediHTTP

to install your project. Jedi already do this. Maybe we should also do this for ycmd.

Reviewed 5 of 5 files at r6. Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

vheon commented 9 years ago

That way the hypothetical package would be installed globally on the user machine, right? And that would assume that the user installing the package has the permission to install the package in the python distribution which I don't know if it always true. If the approach of using pip to install it on a local directory is a valid one we could use it also on ycmd for its submodules (which are all python) and could be even valid for the dependencies of YouCompleteMe which would reduce the download time I guess.

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

I like bundling. It means you have a tried-and-test omnibus configuration that just-works. We have enough trouble with different Python versions and installs. Dependency bundling removes hassle for users and maintainers and testers, even if it means a slightly larger footprint.

Also @vheon is totally right about not wanting to or being able to globally install stuff.

puremourning commented 9 years ago

For example I recently installed gitlab. The installer bundled everything including services like Postgres. Install is 1 rpm -ivh and then gitlabctl reconfigure. Impressive. Compare that to following a list of manual steps to install various dependencies.

Valloric commented 9 years ago

I believe that YCM and ycmd bundling everything and providing a trivial install procedure for most users was critical to its success. Note that ycmd is not a python library but a python application; the user shouldn't have to concern themselves with python plumbing (like pip or setup.py or virtualenv or whatever) just because they want C++ or C# completion. Users shouldn't have to care or know how Python's ecosystem works. We should not have our install process start with "now go learn about pip and virtualenv first."

I've personally been greatly annoyed when the language implementation details of an application that have nothing to do with what that application does make it hard for me to use it. A recent example is teamocil. Amazing app, but I've had a neverending stream of problems setting it up on various machines because of Ruby interpreter and library version issues. And now I have to go learn about rbenv and gems and all sorts of Ruby crap I couldn't possibly care less about.

Everything should work out-of-the-box and we should be moving to a space where install is even easier than it is currently, not harder. We've had enough problems with Python versions and whatnot that I'm considering a Rust rewrite of ycmd. We'd auto-build binaries on every commit that statically link musl libc and then we'd even be agnostic of glibc version incompatibility on older systems (which I've encountered too many times). YCM's install.sh would pull in precompiled bins and that's it. The user would never know we use Rust, they'd just get completely hermetic binary that works on their OS, 6 years old or not.

This is all just an idea for now; it would certainly be a ton of work. Anyway, this PR is not the place to discuss it; I merely wanted to remind everyone that an easy setup process is critical. You've all seen the issue tracker and know that system incompatibilities in setup are a big (if not the biggest) problem for us.

@vheon WRT Flask, sounds like going with Bottle might just make more sense. It's the devil we know.

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

vheon commented 9 years ago

Ok, I will then leave things like they are and stick with Bottle (also because Bottle supports more versions of python). I will look more carefully at the exception_plugin and see if that is really necessary and I will integrate the HMAC plugin that I have written already :+1:

Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

vheon commented 9 years ago

So I've made some changes and the tests for this are all passing. I've removed the exception_plugin since it wasn't really needed and inlined the logging_plugin since it was small enough. I'm currently working on the HMAC Authentication plugin which is giving me some problem due to python3 encoding stuff. What I would like to do is the following:

Review status: 18 of 20 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Sounds great. Reviewing small complete chunks is certainly preferable :)

Review status: 18 of 20 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from the review on Reviewable.io

vheon commented 9 years ago

Review status: 18 of 20 files reviewed at latest revision, 1 unresolved discussion.

jedihttp/handlers.py, line 29 [r4] (raw file): This I'm going to change with the HMAC feature since there there is an actual reason to do it :+1:

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Reviewed 1 of 4 files at r3, 1 of 4 files at r5, 3 of 5 files at r6, 2 of 4 files at r7. Review status: all files reviewed at latest revision, 4 unresolved discussions.

jedihttp/handlers.py, line 14 [r7] (raw file): I totally had to google this :)

According to the PEP:

In Python 2.5, you must enable the new absolute import behavior

If I'm reading that correctly, you only therefore need this if you're using Python 2.5. But I think the earliest supported is Python 2.6 ? Certainly we use multi-line imports in ycmd quite a lot. At least, I have, and I didn't realise that I was introducing a potential incompatibility.

I could totally be missing something here.

jedihttp/handlers.py, line 103 [r7] (raw file): This would probably benefit from some commentary. I had to check the json.dump documentation to work out why it might be required (and having done so I can see that it is trying to return a string representation of any objects passed to json.dumps.

Is this basically the equivalent of what ycmd already does?

jedihttp/handlers.py, line 109 [r7] (raw file): Does str( obj ) raise any type of error? According to the docs, this serialiser should only raise TypeError (apparently)

Comments from the review on Reviewable.io

vheon commented 9 years ago

Review status: all files reviewed at latest revision, 4 unresolved discussions.

jedihttp/handlers.py, line 14 [r7] (raw file): Actually that line in this file isn't needed (anyway I will need that for the HMAC implementation). The thing is that when importing a module from the same directory you simply doimport othermodule and it works... on python 2.7. Instead on python3 you have to be explicit about where to start the search for the module using import .othermodule. When you want to write code in a cross version way you use the future module. In ycmd/YouCompleteMe we do not need that since the versions that we support don't have incompatibilities on this. Although we use the future module in one place https://github.com/Valloric/YouCompleteMe/blob/5a186275a581b04bbdb7001475d946e30d0f80b4/python/ycm/unsafe_thread_pool_executor.py#L9

jedihttp/handlers.py, line 103 [r7] (raw file): I took it straight from ycmd, I just renamed it.

jedihttp/handlers.py, line 109 [r7] (raw file): https://github.com/Valloric/ycmd/pull/144

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Reviewed 1 of 1 files at r8. Review status: all files reviewed at latest revision, 4 unresolved discussions.

jedihttp/handlers.py, line 14 [r7] (raw file): I see - the . is mandatory in python 3. And you have to import from future to write forward-compatible code.

Have I ever mentioned that I love python. Wait, probably not XD

jedihttp/handlers.py, line 103 [r7] (raw file): OK shrug :)

jedihttp/handlers.py, line 109 [r7] (raw file): OK shrug :)

Comments from the review on Reviewable.io

Valloric commented 9 years ago

@vheon I'm not sure I understand your last comment; are you blocked on me on something? What would you like me to do?

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon commented 9 years ago

I just wanted to ask if you guys had other comments on the code till now. If you haven't then I'm going to close this and made a PR on this project for:

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

Valloric commented 9 years ago

There seems to be only a small set of new changes since I last looked at this, and those all seem fine. LGTM from me.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

puremourning commented 9 years ago

Lgtm too

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

vheon commented 9 years ago

Ok, let's close this, thanks guys :+1: