rossant / ipymd

Use the IPython notebook as an interactive Markdown editor
BSD 3-Clause "New" or "Revised" License
483 stars 42 forks source link

using entry_points for registration #55

Closed bollwyvl closed 9 years ago

bollwyvl commented 9 years ago

Initial attempt at implementing suggestions from #35.

Introduces a new setuptools entry_point, ipymd.format, all implementations of which will be loaded at runtime... other behaviors are possible, such as requiring additional config. The entry point spec name should be the name used throughout commands, etc. and has been removed from the respective format file FORMAT_WHATEVER. Additionally, using setuptools extras allows the odf import problem to be generalized, which is a nice perk. The dev version could be installed with dependency_links, but that is dark magic.

I've left the registration business in-place and only replaced the module instrospection: it's as good as anything I've seen if you want to hack on something without creating a real module.

The FormatManager now stores the singleton on itself, and will throw an error if a second instance is created. I'm not wild about this pattern, but it seemed like the quickest way to at least encapsulate the functionality.

On testing

I've basically kicked the tires with markdown, but haven't tried the other formats, figuring it would be better to get something out there.

I can't really see how to get the tests working: even after digging through travis and finding the packages i see there, it still won't make test for me, saying

flake8 ipymd --exclude=ipymd/ext/six.py,ipymd/core/contents_manager.py --ignore=E226,E265,F401,F403,F811
py.test --cov-report term-missing --cov ipymd
usage: py.test [options] [file_or_dir] [file_or_dir] [...]
py.test: error: unrecognized arguments: --cov-report --cov ipymd
Makefile:25: recipe for target 'test' failed
make: *** [test] Error 2

It may help other contributors and myself if your test stuff was self-contained within setup.py, i.e. test_requires, cmdclass.... the makefile, mixed conda/pip stuff in travis, plus the various integrations, makes it a little bit opaque what we're supposed to do after git clone.

README

I attempted to document the new patterns. Also some minor copy editing. I've adopted the convention I've seen elsewhere where a notebook file is lowercase while the Notebook app is uppercase.

bollwyvl commented 9 years ago

Gah, bad starting point. rebasing.

rossant commented 9 years ago

Can you try installing pytest flake8 coverage pytest-cov

rossant commented 9 years ago

It may help other contributors and myself if your test stuff was self-contained within setup.py, i.e. test_requires, cmdclass.... the makefile, mixed conda/pip stuff in travis, plus the various integrations, makes it a little bit opaque what we're supposed to do after git clone.

I'd be pleased to see this improved, and feel free to do it actually if you have time. I'm not a big fan of setuptools and I'm not aware of the best practices but I'm willing to make an effort :)

rossant commented 9 years ago

and thanks for the PR! registering new formats definitely seems nicer now. I'll try to review and merge tomorrow

bollwyvl commented 9 years ago

Can you try installing pytest flake8 coverage pytest-cov

Heh, that had actually failed, as pytest-cov doesn't work with coverage 4, which is what you get by default now, i guess. But it's pretty noisy without odf, so I wanted to clean that up some. test_requires was absurd for installing the odf stuff, so i punted it out to a requirements file, which seems like the most reasonable compromise.

I'm not a big fan of setuptools

Yeah, setuptools is the beast that everyone loves to hate. But I haven't seen a compelling replacement that doesn't use setuptools under the covers, so i figure it's worth knowing how to use it.

Most recent push also moves the pytest execution to setup.py test, with the configuration options in setup.cfg to DRY it up a tick.

Love to here what else you think!

rossant commented 9 years ago

Heh, that had actually failed, as pytest-cov doesn't work with coverage 4, which is what you get by default now, i guess.

ughh

Yeah, setuptools is the beast that everyone loves to hate. But I haven't seen a compelling replacement that doesn't use setuptools under the covers, so i figure it's worth knowing how to use it.

Fair enough. Seeing https://pythonhosted.org/setuptools/setuptools.html makes me want to cry, so I'm more than glad to benefit from your help :)

rossant commented 9 years ago

Looks very good to me. Ready to merge on my end once we decide what to do with conda (but we can tackle this in another PR too)

bollwyvl commented 9 years ago

If the code looks fine, merge, by all means! Tests run snappily, and even the other changes, not related to registration may have been out of scope.

I love conda, great piece of work, use it every day... that i use Windows. I think since jupyter caters to the scipy crowd, it's worth testing there, especially if the tests are faster... Is it worth maintaining an environment.yml, too? Honestly i should start using it in linux, if only to save bandwidth and battery. :)

Does conda constitute a sufficient test for the vanilla pip environment? Maybe not. Maybe wait and see. I would certainly test every release... Pip install ipmyd ipython[notebook] seems valuable!

On 05:15, Sun, Jun 14, 2015 Cyrille Rossant notifications@github.com wrote:

Looks very good to me. Ready to merge on my end once we decide what to do with conda (but we can tackle this in another PR too)

— Reply to this email directly or view it on GitHub https://github.com/rossant/ipymd/pull/55#issuecomment-111805168.

rossant commented 9 years ago

great, thanks! merged.

I do test pip install before releasing (I have a docker image that does that). However I cannot easily run the tests because the static files (Markdown, ODT, etc.) required for the tests are not part of the package -- do you feel they should be part of it?

bollwyvl commented 9 years ago

huzzah!

i think keeping the package light is great. package the first ODT and boom, double the size.

if you already have a process for validating pip success that is working for you, no need to change! i have run into problems only once it is actually pulling from pip against a clean cache... maybe tox would help me with clean environments. I am trying to learn more about that end of packaging, having mostly concentrated on in-house stuff where i had more control but fewer assets.

I love docker as an end-to-end test: I really like how IPython ships their docker image with the last thing run being the tests.

I imagine some folk might like to use said dockerfile... i see there is a odewahn/ipymd, but an official build from master would be lovely.

On Sun, Jun 14, 2015 at 11:04 AM Cyrille Rossant notifications@github.com wrote:

great, thanks! merged.

I do test pip install before releasing (I have a docker image that does that). However I cannot easily run the tests because the static files (Markdown, ODT, etc.) required for the tests are not part of the package -- do you feel they should be part of it?

— Reply to this email directly or view it on GitHub https://github.com/rossant/ipymd/pull/55#issuecomment-111838615.

rossant commented 9 years ago

i think keeping the package light is great. package the first ODT and boom, double the size.

I also like to keep packages (and repos!) as light as possible. However the ODT files are a couple of KB each, so I don't think that's a problem.

if you already have a process for validating pip success that is working for you, no need to change

Well I can only check that import ipymd works in a Python console, but I cannot run the tests because the Docker image doesn't have the example files. I guess I could add them somehow but it might be simpler to just include the examples in the package (in ipymd/static or something...). Also, users might want to run the tests themselves with e.g. ipymd.test().

I imagine some folk might like to use said dockerfile... i see there is a odewahn/ipymd, but an official build from master would be lovely.

Yeah I think if I add a Dockerfile in master I can have Dockerhub rebuild the image automatically, which would be great.