Skip to content

Conversation

@Abelarm
Copy link
Contributor

@Abelarm Abelarm commented Oct 20, 2020

Issue ref: #3905

Added a function to setup.py for downloading the badge locally and replacing the url with the local path.

Fixes #3914
Fixes #3905

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

SURE!

@pep8speaks
Copy link

pep8speaks commented Oct 20, 2020

Hello @Abelarm! Thanks for updating this PR.

Line 68:121: E501 line too long (145 > 120 characters)

Comment last updated at 2020-12-01 21:07:29 UTC

@mergify mergify bot requested a review from a team October 20, 2020 10:57
@Abelarm
Copy link
Contributor Author

Abelarm commented Oct 20, 2020

I have no idea how to fix the issue with CodeFactor :(

I can't use Requests how suggested by CodeFactor since is the setup and no external should be used (at least I think)

@codecov
Copy link

codecov bot commented Oct 20, 2020

Codecov Report

Merging #4250 (58d246a) into master (e952dee) will decrease coverage by 0%.
The diff coverage is 91%.

@@          Coverage Diff           @@
##           master   #4250   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         123     124    +1     
  Lines        9255    9320   +65     
======================================
+ Hits         8581    8640   +59     
- Misses        674     680    +6     

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great addition for the doc. Would you mind testing those 2 functions ?

setup.py Outdated
return reqs


def _parse_README_for_badge(text):
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance you could add tests for those functions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaton I am having a problem:

I wanted to create a test for the two functions, but since they are inside the setup.py file when i try to import them it runs the setup itself, and in this way it crash...how do you think I should proceed?

  • Move the two functions outside the setup, but where?
  • Modifying setup in someway that prevents its run when function from it are imported?

@ydcjeff
Copy link

ydcjeff commented Oct 20, 2020

Not sure this will work on pypi page, but we can get .png from shields.io by replacing img with raster
See: https://raster.shields.io/pypi/pyversions/pytorch-lightning

@mergify mergify bot requested a review from a team October 20, 2020 14:36
@Abelarm
Copy link
Contributor Author

Abelarm commented Oct 20, 2020

Not sure this will work on pypi page, but we can get .png from shields.io by replacing img with raster
See: https://raster.shields.io/pypi/pyversions/pytorch-lightning

that's exactly what my script tries to do, first it asks for the .png version by adding the file format (eg. .png) at the end of the url, if the page responds with a valid png then it save it otherwise it downloads the .svg version.
But it always try to download the png version first

@Abelarm Abelarm requested review from tchaton and removed request for a team October 21, 2020 14:28
@mergify mergify bot requested a review from a team October 21, 2020 14:29
@stale
Copy link

stale bot commented Nov 4, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label Nov 4, 2020
@Borda Borda added docs Documentation related feature Is an improvement or enhancement and removed won't fix This will not be worked on labels Nov 4, 2020
@Borda Borda self-assigned this Nov 4, 2020
@Borda
Copy link
Collaborator

Borda commented Nov 30, 2020

@tchaton @SeanNaren can you chek, it shall be fine...

@Borda Borda self-requested a review November 30, 2020 19:51
@Borda
Copy link
Collaborator

Borda commented Nov 30, 2020

@Abelarm thinking if we can add simple doctests to for the parsing, just add synthetic sample code and or just a line from readme and see what it return...

@Borda Borda added this to the 1.1 milestone Nov 30, 2020
@Abelarm
Copy link
Contributor Author

Abelarm commented Dec 1, 2020

@Abelarm thinking if we can add simple doctests to for the parsing, just add synthetic sample code and or just a line from readme and see what it return...

OK I'll work on it.

@Abelarm
Copy link
Contributor Author

Abelarm commented Dec 1, 2020

Hi @Borda, I'm seeing a lot fo commits, is there something I can help you with?

@Borda
Copy link
Collaborator

Borda commented Dec 1, 2020

Hi @Borda, I'm seeing a lot fo commits, is there something I can help you with?

just finalizing and catching some last issues... like Win has problems with re.sub(...) expresion

@Borda
Copy link
Collaborator

Borda commented Dec 1, 2020

one more todo would be better determining the badge type, eg in the list of badge names also assign what is the type, if SVG or PNG...

@Borda Borda added the ready PRs ready to be merged label Dec 1, 2020
@Borda Borda merged commit a941f96 into Lightning-AI:master Dec 1, 2020
Borda added a commit that referenced this pull request Dec 2, 2020
… url with downlaod path (#4250)

* Added the function for downloading the badge locally, replacing the url

* Fixed the pep8 errors, pointed out during pull request

* Update setup.py

* refactor

* format

* test

* Added Doctest on the functions

* test

* test

* fix

* format

* fix

* fix

* prune

* fiix

* fiix

* flake8

* fix

* imports

* imports

* imports

* fixx

* impoets

* win

* min

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Jirka Borovec <[email protected]>
@Borda
Copy link
Collaborator

Borda commented Dec 2, 2020

@Abelarm it seems that it did not work for PyPI page
see our today's RC - https://pypi.org/project/pytorch-lightning/1.1.0rc1
when I open the PKG-INFO I still see the original paths...

@Abelarm
Copy link
Contributor Author

Abelarm commented Dec 9, 2020

@Abelarm it seems that it did not work for PyPI page
see our today's RC - https://pypi.org/project/pytorch-lightning/1.1.0rc1
when I open the PKG-INFO I still see the original paths...

Hi Borda,

sorry was out for vacation.
I can see that you already solve it. Is there anything else I can do to help you regarding this?

Best, Luigi

@Borda
Copy link
Collaborator

Borda commented Dec 9, 2020

well, there is still Drone that does not have past version states...

@Abelarm
Copy link
Contributor Author

Abelarm commented Dec 10, 2020

Sorry I didn't get exactly what is the problem, and hence how I am supposed to solve it :(

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

Labels

docs Documentation related feature Is an improvement or enhancement priority: 1 Medium priority task ready PRs ready to be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

copy badges to release package

6 participants