Make styles able to be extended by users #103

Merged
merged 3 commits into from Feb 6, 2017

Projects

None yet

3 participants

@dahlia
Contributor
dahlia commented Jan 30, 2017

This patch replace hardcoded style names with setuptools' entry points.

Entry points are a simple way for distributions to “advertise” Python objects (such as functions or classes) for use by other distributions. Extensible applications and frameworks can search for entry points with a particular name or group, either from a specific distribution or from all active distributions on sys.path, and then inspect or load the advertised objects at will.

The group name of extensible entry points is flake8_import_order.styles (which follows the convention). Built-in styles also become discovered using entry points so that style names don't have to be hardcoded.

Also wrote some docs about extending styles into README.rst, but I'm unsure that my English is fine. If anyone reviews my code and docs I would adjust the patch.

Thanks for the very useful program! 👍

@dahlia
Contributor
dahlia commented Jan 30, 2017

The build was broken due to setuppy, but it seems the same to the latest master build. And it's not reproduced in my local environment…

setuppy create: /home/travis/build/PyCQA/flake8-import-order/.tox/setuppy
setuppy installdeps: docutils, Pygments
setuppy inst: /home/travis/build/PyCQA/flake8-import-order/.tox/dist/flake8-import-order-0.11.zip
setuppy installed: appdirs==1.4.0,docutils==0.13.1,flake8-import-order==0.11,packaging==16.8,pycodestyle==2.2.0,Pygments==2.2.0,pyparsing==2.1.10,six==1.10.0
setuppy runtests: PYTHONHASHSEED='2730747044'
setuppy runtests: commands[0] | python setup.py check --metadata --restructuredtext --strict
running check
warning: check: Could not finish the parsing.
error: Please correct your package.
ERROR: InvocationError: '/home/travis/build/PyCQA/flake8-import-order/.tox/setuppy/bin/python setup.py check --metadata --restructuredtext --strict'
tox.ini
-deps = docutils
+deps =
+ docutils
+ Pygments
@sigmavirus24
sigmavirus24 Jan 30, 2017 Member

Why is Pygments here?

@dahlia
dahlia Jan 30, 2017 Contributor

@sigmavirus24 Because I added .. code-block:: python directive to README.rst. Docutils warns if Pygments is not installed.

@sigmavirus24
sigmavirus24 Jan 30, 2017 Member

I don't think PyPI supports .. code-block:: python so I'm not sure it should be used in the readme.

@dahlia
dahlia Feb 2, 2017 Contributor

@sigmavirus24 PyPI supports .. code-block:: directive and it's even colored. 😄 GitHub also supports it and it's colored too.

@sigmavirus24

So pkg_resources as well as iter_entry_points are expensive. Calling them more times than is necessary seems wasteful. I think what you should do is retrieve all of them once and store away the style entry-point before running any checks.

- help=("Style to follow. Available: "
- "cryptography, google, smarkets, appnexus, pep8"),
+ help=("Style to follow. Available: " +
+ ", ".join(cls.list_available_styles())),
@sigmavirus24
sigmavirus24 Jan 30, 2017 Member

These should be sorted otherwise they'll never be displayed the same way twice.

flake8_import_order/flake8_linter.py
+ )
+ for entry_point in entry_points:
+ yield entry_point.name
+
@classmethod
def parse_options(cls, options):
@sigmavirus24
sigmavirus24 Jan 30, 2017 Member

You should be finding the entry-point here and passing it through on the options object.

flake8_import_order/checker.py
- style = Edited(imports)
- else:
+ try:
+ style_entry_point = next(
@sigmavirus24
sigmavirus24 Jan 30, 2017 Member

You can probably pass the entry-point through to here on the options object so you don't have to delay style loading until here. This also means you're not loading it for each and every file.

@dahlia dahlia Eagerload entry point & sort style names
Adjusted a way to load style entry points, and made
available style names (in --import-order-style help)
sorted.

PyCQA#103 (review)
87ea879
@dahlia
Contributor
dahlia commented Jan 30, 2017

@sigmavirus24 I made it to load a style entry point before ImportOrderChecker is instantiated. Made available style names ordered as well.

@dahlia
Contributor
dahlia commented Feb 1, 2017 edited

What things would there be to change more?

@sigmavirus24
Member

@dahlia I don't merge on this repository. That's @pgjones call. I'll dismiss my review since GitHub's review system is so broken. I would suggest looking at the Travis-CI failures though.

@sigmavirus24 sigmavirus24 dismissed their review Feb 1, 2017

Feedback was addressed

@pgjones
Member
pgjones commented Feb 1, 2017

Do you have a use case in mind for this? I'm not sure what the potential uptake is, and how to weigh it against the extra complexity. (I can't take a detailed look until next week, sorry).

@dahlia
Contributor
dahlia commented Feb 2, 2017

My motivation was to implement some private rules of my work without changing flake8-import-order's code or maintaining its fork. I believe there would be other companies than my work which have their own modified rules of PEP 8 for some rationale.

@sigmavirus24
Member

Adding onto the use-case discussion:

This would mean that new styles need not be added directly to this plugin. People can write other plugins that they distribute separately and which can be installed. Someone could then pip install this plugin and say flake8-import-order-twisted or flake8-import-order-django.

@dahlia dahlia Fix broken setuppy build in Travis CI
https://travis-ci.org/PyCQA/flake8-import-order/builds/196644226

Failure of setuppy build is due to a bug of distutils.command.check [1]
which fixed at CPython 2.7.10 [2].

Since Travis CI provides CPython 2.7.9, I changed setuppy build
to use Python 3.5 instead to workaround CPython 2.7.9's bug.

[1] https://bugs.python.org/issue23063
[2] https://hg.python.org/cpython/rev/38826e21f0db
0d34a82
@dahlia
Contributor
dahlia commented Feb 2, 2017 edited

Failure of tox -e setuppy build was due to a bug of distutils.command.check, and it was fixed at CPython 2.7.10. Since Travis CI provides CPython 2.7.9, I changed tox -e setuppy to use Python 3.5 instead to workaround CPython 2.7.9's bug.

@pgjones
Member
pgjones commented Feb 6, 2017

Ok, I'm convinced and this looks good to me. I'll merge and make a few tweaks before releasing.

@pgjones pgjones merged commit 6ab9653 into PyCQA:master Feb 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pgjones pgjones added a commit that referenced this pull request Feb 6, 2017
@dahlia @pgjones dahlia + pgjones Eagerload entry point & sort style names
Adjusted a way to load style entry points, and made
available style names (in --import-order-style help)
sorted.

#103 (review)
b2ae1da
@dahlia
Contributor
dahlia commented Feb 6, 2017

Thanks for merging! 🙏

@dahlia dahlia referenced this pull request in spoqa/spoqa.github.com Feb 12, 2017
Merged

flake8-import-order-spoqa 소개글 #190

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment