-
Notifications
You must be signed in to change notification settings - Fork 14
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
thanks nows read data from the packages and json-api (even tho the data is missing) #23
Conversation
Hoping to look at this PR in more depth later today. |
True, but even |
🤦♂️ I guess I was a fool to assume it would appear in the json-api results. Any idea where the code base which serves the json-api lives? However, it does extract the url from local installs. |
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.
Overall I think this looks good, minus some nits.
Also, I think tests are failing because ddt needs to be added to test_requires
or in some other way referenced in setup.py
thanks/package_tools.py
Outdated
return ( | ||
metadata['extensions']['python.details']['project_urls']['Funding'] | ||
) | ||
print(metadata) |
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.
Is this leftover debugging code?
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/package_tools.py
Outdated
# Dist filename syntax | ||
# name ["-" version ["-py" pyver ["-" required_platform]]] "." ext | ||
# https://setuptools.readthedocs.io/en/latest/formats.html#filename-embedded-metadata | ||
filename = ''.join(chain(*takewhile(lambda x: x[1], ( |
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 looks cool as hell; could you add a comment explaining what this is doing? These are parts of itertools I'm not familiar with
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.
🤔 if it needs a comment maybe I'm trying to be too clever.
Basically it takes all components parts of the filename until it finds a None
then concatenates them into the finished filename.
Will refactor to try and make it self-explanatory and cognitively easy on the reader (which will probably be me sometime in the future)
Corrected linting errors and removed rouge 'print' calls. Moved to using nosetest as the setup.py test runner. This is mainly as nose give clearer error messages when something goes wrong. Also of note there were pinned dependencies which have been changed to 'at least' version dependencies. For reasons why see https://caremad.io/posts/2013/07/setup-vs-requirement/ Laster, we currently have a README.md and a README.rst. I'm unsure which one of these we mean to use. Shouldn't we at the very least keep all of the following in the same markup format: README, HISTORY, CONTRIBUTING, AUTHORS,
Quick search seems to indicate that we'd have to expose the funding url in the json-api here I was about to suggest looking in the xmlrpc data but then encountered this:
|
Just found pypi/warehouse#3831 which will probably work for us once it's completed |
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.
Overall this looks great. I have a question about using nose
, but once we've chatted about that, ready to approve.
@@ -60,7 +62,7 @@ | |||
'Programming Language :: Python :: 3.5', | |||
'Programming Language :: Python :: 3.6', | |||
], | |||
test_suite='tests', | |||
test_suite='nose.collector', |
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.
Could you talk a bit about why we're moving to nose
?
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.
There was a small note in the commit.
Basically python setup.py test
was swallowing some simple errors (such as that the requests package wasn't installed.
nose
presented these errors clearly and reminded me of my obvious error.
return None | ||
|
||
|
||
metadata_patterns = re.compile(r""" |
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 is a cool little collection of regexes, nice.
@@ -61,15 +53,29 @@ def _display_thanks(self): | |||
horizontal_bar=' ', | |||
vertical_bar=' ', | |||
)) | |||
cprint( | |||
''.join([ | |||
"If you see projects with ", |
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 notice is lovely, and nicely viral in growing the packages which will have funding info.
@phildini I think this is most of the way to addressing #7 albeit a bit rough around the edges.
Of note:
thanks.json
PKG-INFO
for eggs orMETADATA
for distsI've yet to find a project on PyPI which contains a funding url so, although I believe it all works, I haven't been able to confirm against the live wharehouse.