search

vheon / JediHTTP

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

Move jedihttp.py into jedihttp/ #18

Closed yan12125 closed 8 years ago

yan12125 commented 8 years ago

It's better not to put Python scripts at the top level. In this PR I keep /jedihttp.py until ycmd has changed, too.

This change is Reviewable

vheon commented 8 years ago

It's better not to put Python scripts at the top level

Care to elaborate more? Or point me to some documentation explaining why? Because I remember that if I didn't do it like this the tests where I spawn a JediHTTP instance would fail.

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

_Comments from Reviewable_

yan12125 commented 8 years ago

From https://packaging.python.org/distributing/:

"Although it’s not required, the most common practice is to include your python modules and packages under a single top-level package that has the same name as your project, or something very close."

if I didn't do it like this the tests where I spawn a JediHTTP instance would fail.

Should be fixed in 24e2288.

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from [Reviewable](https://reviewable.io:443/reviews/vheon/jedihttp/18#-:-KLaJ4bZLifLd23TtAc:-1117433923)_

vheon commented 8 years ago

Should be fixed in 24e2288.

It isn't. Full disclosure: in the first place I followed the "common practice" but then the tests failed and since I couldn't find a way to change the tests to make this working I change the way the project is structured. I'm absolutely a Python expert so if you can make the tests working I'm not against changing the structure of the project.

Although it’s not required, the most common practice

So the way it is right now is not wrong but since is a common practice I expect to be some advantages to do it that way but I don't see them, and that is why I asked. Furthermore currently I don't intend to distribute this in the "standard" Python way so I don't know how much sense would make to follow those guide but I am more than willing to change it if I see some benefits other than "everyone do it like that".

Review status: 0 of 2 files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

yan12125 commented 8 years ago

It was my fault - I have another JediHTTP installation on my machine. Now both Travis CI and AppVeyor passed.

A concrete reason is for #17 - a single directory makes setup.py simpler.

Comments from [Reviewable](https://reviewable.io:443/reviews/vheon/jedihttp/18#-:-KLbqbDrt-uQrmvjjxj:2109332817)_

vheon commented 8 years ago

If I have to be honest the first thought of the jedihttp/__main__.py file was that it was a cheat to just makes things working and not a "real" solution... but what would I know about Python? :stuck_out_tongue_closed_eyes:

Can you remove the /jedihttp.py file? ycmd is still using the current latest commit anyway so it would not matter to it ;)

Honestly I have to think about this some more because I'm still not convinced :confused:

I really appreciate the PR though :+1:

Reviewed 1 of 1 files at r2, 3 of 5 files at r3, 1 of 1 files at r4. Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.

jedihttp/main.py, line 5 [r4] (raw file):


if __package__ is None:
    sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__))))

Spaces inside the parentheses. Also couldn't we write this as:

sys.path.append( path.abspath( path.join( __file__, '..', '..' ) )

Comments from Reviewable

yan12125 commented 8 years ago

Here's a question: do ycmd and JediHTTP still support Python 2.6? If not ycmd can just call python -m jedihttp and the __package__ hack can be removed.

Comments from Reviewable

vheon commented 8 years ago

Here's a question: do ycmd and JediHTTP still support Python 2.6? If not ycmd can just call python -m jedihttp and the package hack can be removed.

yes we do; at least on Linux.

Review status: 5 of 7 files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

yan12125 commented 8 years ago

_jedihttp/main.py, line 5 [r4] (raw file):_

Previously, vheon (Andrea Cedraro) wrote… > Spaces inside the parentheses. Also couldn't we write this as: > > ``` python > sys.path.append( path.abspath( path.join( __file__, '..', '..' ) ) > ``` > >

Thanks. Changed.

Comments from Reviewable

yan12125 commented 8 years ago

Closing - found a way to have a working setup.py without put everything into a single directory. See #19.