Skip to content
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

Add data-requires-python attribute to file links (PEP 503) #56

Merged
merged 3 commits into from
Jul 22, 2016

Conversation

takluyver
Copy link
Contributor

This was suggested by @dstufft in pypa/pip#3847

@dstufft
Copy link
Member

dstufft commented Jul 14, 2016

Just to be clear, you should post this change on distutils-sig to give folks a chance to comment on it there, and probably we should add a note at the bottom of the PEP to say it was adjusted on X date to add data-requires-python or so.

**SHOULD** ignore the download when installing to a Python version that
doesn't satisfy the requirement. For example::

<a href="..." data-requires-python=">=3">...</a>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth adding/using a <= example, as that needs encoding as &lt .

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< needs to be encoded, but > doesn't? I'm having some trouble finding info on this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

< needs encoding in every flavour of HTML/SGML/XML/etc.
> needs to be encoded for some of them (sorry I dont recall the specifics offhand), but not all of them, so generally people and tools encode it all the time, to be safe and future proof in case the DOCTYPE of the document is changed in the future, or a browser implicitly detects it as a different DOCTYPE than the writer intended.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - I've updated the example and added a note indicating that they should both be encoded. Better safe than sorry.

@Carreau
Copy link
Contributor

Carreau commented Jul 14, 2016

if a project have the requires-python or any other informations that state that this link is only compatible with specific python version, maybe the repository SHOULD include these attributes ?

@warsaw
Copy link
Member

warsaw commented Jul 14, 2016

I'm just curious. If this is exposing the Requires-Python metadata, why the data- prefix on data-requires-python?

@dholth
Copy link
Contributor

dholth commented Jul 14, 2016

Works for me.

Data- is a html5 thing

@takluyver
Copy link
Contributor Author

AIUI data- is a namespace where you can add arbitrary attributes and still have valid HTML.

@ncoghlan
Copy link
Contributor

The basic idea seems sound to me, but I'll second @jayvdb's suggestion to clarify the requirements around escaping greater-and-less-than signs (it may not be a problem since they're inside a quoted string regardless, but the PEP should say either MUST or MUST NOT one way or the other, so that consumers know whether or not they need to undo any HTML quoting)

Also +1 to @dstufft observation that the update should be noted towards the end of the PEP. It would also be desirable to mention the update in https://packaging.python.org/specifications/#simple-repository-api (although that can just link to the note in the PEP itself)

@takluyver
Copy link
Contributor Author

I've added a 'changes' section near the end of the PEP with a note of this.

I'm happy to add something about escaping, but I'm unsure what escaping is required. This SO answer seems to agree with what @jayvdb suggested, that < needs encoding but > doesn't, but I can't find any canonical information, and other sources seem to say different things.

@brettcannon
Copy link
Member

If you can't find consistent information then it's probably best to be safe and just escape the whole string.

@takluyver
Copy link
Contributor Author

I have now added a note that < and > should both be escaped. Does that seem reasonable?

@takluyver
Copy link
Contributor Author

Does anyone want any more changes to this?

@dstufft
Copy link
Member

dstufft commented Jul 22, 2016

Forgive me because I can't recall- Was this mentioned on distutils-sig at all? I seem to believe it was but I don't have it up in front of me at the moment.

@takluyver
Copy link
Contributor Author

Yep: https://mail.python.org/pipermail/distutils-sig/2016-July/029255.html

Two people said 'LGTM' there, and it probably drove a few comments here.

@dstufft
Copy link
Member

dstufft commented Jul 22, 2016

Okay! Just wanted to make sure everyone had a chance to comment. I didn't think there'd be much push back on this :)

@Carreau
Copy link
Contributor

Carreau commented Jul 22, 2016

Forgive me because I can't recall- Was this mentioned on distutils-sig at all? I seem to believe it was but I don't have it up in front of me at the moment.

If was, here,

with at the time of this writing 2 responses:

Daniel Holth:
Lgtm

Xavier Fernandez
LGTM also :)
For the record, I've started a related PR to check Requires-Python in pip
at install time: pypa/pip#3846
It should still be useful once PEP 503 is amended, for packages or package
indexes that could not expose the Requires-Python information via this new
mecanism.

@dstufft
Copy link
Member

dstufft commented Jul 22, 2016

I filed an issue with Warehouse to make sure it gets implemented there, I'm not going to implement it in legacy but if someone else does I'm happy to review it and merge it (also not going to implement it in Warehouse today, but if someone feels like doing that, happy to review and merge that as well).

@Carreau
Copy link
Contributor

Carreau commented Jul 22, 2016

I filed an issue with Warehouse to make sure it gets implemented there, I'm not going to implement it in legacy but if someone else does I'm happy to review it and merge it (also not going to implement it in Warehouse today, but if someone feels like doing that, happy to review and merge that as well).

When will pip switch to warehouse by default ? (or did it have already)

@dstufft
Copy link
Member

dstufft commented Jul 22, 2016

When will pip switch to warehouse by default ? (or did it have already)

When Warehouse is ready, we don't have a date :/

@Carreau
Copy link
Contributor

Carreau commented Jul 22, 2016

When Warehouse is ready, we don't have a date :/

Or I'll see if I figure out a fix for Legacy PyPI.

@takluyver
Copy link
Contributor Author

Cross referencing: pypa/pip#3877 adds support for this in pip.

@takluyver takluyver deleted the data-requires-python branch July 30, 2016 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants