-
Notifications
You must be signed in to change notification settings - Fork 139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement extras_require #181
Conversation
Thanks, I think this makes sense. Some thoughts coming up... |
flit/common.py
Outdated
dev_requires = () | ||
extras_require = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metadata 2.0 was dropped, and metadata 2.1 is much closer to the 1.x formats. This metadata class is meant to closely represent the metadata we actually write/upload, so I'd rather add the extra requirements to requires_dist
rather than adding a dict attribute to it.
The _prep_metadata()
function in flit.inifile
is responsible for converting the metadata we read from the file to a dict representing the standardised Python metadata.
doc/pyproject_toml.rst
Outdated
|
||
These are not (yet) encoded in the wheel, but are used when doing | ||
``flit install``. | ||
These are installed by ``flit install`` and encoded in the wheel as extra ``dev``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make this undocumented (or maybe leave it for now but mark it as deprecated). The canonical way will become extras-require.dev
.
Other stuff that I think this should have:
Also, one thing I'm still thinking about - input welcome: I don't like the name |
I added some of your recommendations and will get to the others eventually |
OK, all done, but I have more ideas for tests:
|
OK, done! now i’m happy with it. |
OK @takluyver now i’m really happy! could you please check this? |
This doesn't seem to work for me. I pulled this PR and installed it locally, by doing Then I did I have the following in the other package's pyproject.toml
When I ran Using Python 3.6.5 if it matters. |
the two reserved extras ``test`` and ``doc`` as well as the extra ``dev`` | ||
are installed by ``flit install``. For example: | ||
|
||
.. code-block:: toml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will emit sphinx warning message: WARNING: Pygments lexer name 'toml' is not known
.
I'm using Sphinx 1.7.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we could add pygments-github-lexers
to the doc
extra requirements
That’s weird! the tests do work after all. There must be something I missed 🤔 What does |
|
Hmm, before I debug myself, could you please quickly try with the newest commit? Please also try |
I tried both
|
OK, so what I missed is that I fixed it, and it works now! |
Thanks!
|
Fixed, that was an oversight! Thank you for helping, that was a bigger PR than anticipated |
Awesome! Thank you! (I didn't look at your code). |
Great! Now I also added the In order to get the TOML highlighting in the docs as well, we need to install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience, I know I've been slow to get round to reviewing this. The actual implementation is looking good now, but there are still a few changes I'd like.
doc/pyproject_toml.rst
Outdated
dev-requires | ||
Packages that are required for development. This field is in the same format | ||
as ``requires``. | ||
extras-require |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the user facing name for this field to be requires-extra
or requires-extras
(I don't have a strong preference between these two - thoughts?). I like the symmetry between requires
, requires-python
and requires-extra[s]
.
I think of this like "the package requires these packages, this version of Python, and optionally these further packages for extra features."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires-extras
and requires-extra
imply that the package requires the extras. However it’s the extra features that require certain dependencies. So only extras-require
says the right thing, don’t you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't. The naming extras_require
has always felt unintuitive to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, requires-extra
makes a little more sense, I’m going for that one then.
tests/test_inifile.py
Outdated
inf = read_pkg_ini(samples_dir / 'module1-pkg.toml') | ||
assert inf['module'] == 'module1' | ||
assert inf['metadata']['home_page'] == 'http://github.com/sirrobin/module1' | ||
|
||
def test_misspelled_key(): | ||
def test_misspelled_key(samples_dir): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand what you've done here, and I'm a big fan of py.test's capabilities, but in this case I prefer the simplicity of a global variable that the test functions use over the wizardry of a py.test fixture. Sorry, but I'm going to ask you to revert all these samples_dir
changes.
@@ -82,7 +82,8 @@ Install the package on your system. | |||
.. option:: --deps <dependency option> | |||
|
|||
Which dependencies to install. One of ``all``, ``production``, ``develop``, | |||
or ``none``. Default ``all``. | |||
or ``none``. ``all`` and ``develop`` install the extras ``test``, ``docs``, | |||
and ``dev``. Default ``all``. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to document the new --extras
option too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Thanks! Now I think the last bit that needs to be done is adding the |
done! |
Thank-you :-) |
No problem 😀 |
Fixes #144