-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Query objects.inv from homepage url #40
base: master
Are you sure you want to change the base?
Conversation
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'm OK with the changes in principle, I just have a few comments.
raise ValueError(f"objects.inv not found at url {objects_inv_url}: HTTP Status {r.status_code}.") | ||
|
||
return docs_url | ||
stderr_writer(f"WARNING: objects.inv not found at url {objects_inv_url}: HTTP Status {r.status_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.
I'm reluctant to accept this change as get_sphinx_doc_url
is a public function that other modules can use to query documentation URLs. I'd like the caller to still be able to catch the ValueError
and display the message to the user themselves.
If you're wanting to make the warning on line 197 more specific -- to say why the documentation URL couldn't be obtained -- how about including the exception message text? E.g.:
except (ValueError, requests.exceptions.ConnectionError, requests.exceptions.Timeout) as e:
# Couldn't get it from PyPI, trying fallback mapping
if project_name in fallback_mapping():
doc_url = fallback_mapping()[project_name]
intersphinx_mapping[project_name] = (doc_url, None)
else:
stderr_writer(f"WARNING: Unable to determine documentation url for project {project_name}: {str(e)}")
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 your point, removing the raise
was here to ensure all URLs are queried until one is good.
Due to the addition of the home-page URL in docs_urls
, they may be more than one URL to iterate over. Keeping the raise
makes get_sphinx_doc_url
always return or fail on the first one, so if the documentation URL is bad, but homepage is good, it will not work.
Pull Request Test Coverage Report for Build 1983852400Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
7b558fd
to
db8e658
Compare
Problem
On PyPi, it's very common that the 'Homepage' url links to the documentation.
Proposal
Add the homepage URL to the project links