Skip to content

Change pep command to use .txt by default #378

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

Merged
merged 2 commits into from
Oct 14, 2019
Merged

Change pep command to use .txt by default #378

merged 2 commits into from
Oct 14, 2019

Conversation

larswijn
Copy link
Contributor

@larswijn larswijn commented Jul 1, 2019

The pep command would assume either .txt or .rst, which was not always correct. It now always tries both, and should still error the same way when the pip was not found. This is the 2nd time this pull request has been opened because the write history of the previous one had been destroyed.

Closes: #367

to fix the write history of the file
@jchristgit
Copy link
Member

Couldn't we first try to fetch it by the current logic:

        if pep_number > 542:
	            pep_url = f"{self.base_github_pep_url}{pep_number:04}.rst"
	        else:
	            pep_url = f"{self.base_github_pep_url}{pep_number:04}.txt"

and fall back to the other filetype if one returns 404?

@jchristgit jchristgit self-assigned this Jul 20, 2019
@MarkKoz MarkKoz added area: cogs t: bug Something isn't working labels Aug 6, 2019
@larswijn
Copy link
Contributor Author

You could, but... I personally don't see a reason to do so:
Up until pep 542 almost all of the peps are in .txt format, and after pep 542, they're equally split between .rst and .txt. It would be useful to start with the .txt extension first, since that one comes up a lot more often, which would be a simple case of switching around the order that's tried which is just a list in the code.

@lemonsaurus
Copy link
Member

Yeah, it's weird that we're trying it in the wrong order currently. Sounds like the right approach is:

  1. try .txt
  2. If and only if this fails, try .rst

this would lead to far fewer requests, but it would take twice as long to resolve the .rst ones because of 2 sequential requests. However, I don't think it'll take so long that most people will notice or mind, so I think that's fine.

@scragly scragly changed the base branch from django to master September 15, 2019 08:58
@scragly scragly changed the title Change get_pip command regarding file extensions #2 Change pep command to use .txt by default Sep 17, 2019
@scragly
Copy link
Contributor

scragly commented Sep 17, 2019

@larswijn Would you like to revisit this if you get some time.

Switch around trying order (txt first, then rst)
@larswijn
Copy link
Contributor Author

Updated, should be finished now.

@jchristgit jchristgit merged commit 21bdeb8 into python-discord:master Oct 14, 2019
@larswijn larswijn deleted the django branch July 7, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PEP command incorrectly assumes file extension type
6 participants