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

BUG: Include port number if present #582

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

chmmao
Copy link
Contributor

@chmmao chmmao commented Aug 5, 2024

This is a quick fix to address the Issue #563, by including the port number if present
in the provided URL.

The code checks if the baseurl has port number, and adds to the hostname in curated_baseurl.
This should fix the issue that the port number was missing as #563 described.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Thank you for the PR.

The fix works as expected, however, could you please add a test for this? The code snippet from the bug report issue should suffice.

pyvo/dal/tests/test_tap.py Outdated Show resolved Hide resolved
@bsipocz bsipocz changed the title [Issue 563] Include port number if present BUG: Include port number if present Oct 1, 2024
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.70%. Comparing base (cc17425) to head (2026a5f).
Report is 110 commits behind head on main.

Files with missing lines Patch % Lines
pyvo/dal/vosi.py 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #582      +/-   ##
==========================================
+ Coverage   81.68%   81.70%   +0.01%     
==========================================
  Files          69       69              
  Lines        7089     7093       +4     
==========================================
+ Hits         5791     5795       +4     
  Misses       1298     1298              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

Overall looks good. I push the fstrings, the style check fixes and a changelog entry in a new commit and then will go ahead with merging this.

Thank you @chmmao!

pyvo/dal/vosi.py Outdated
endpoint=endpoint),

return [
'{baseurl}/{endpoint}'.format(baseurl=curated_baseurl, endpoint=endpoint),
Copy link
Member

Choose a reason for hiding this comment

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

Use fstring here, too.

pyvo/dal/vosi.py Outdated
netloc = urlcomp.hostname
if urlcomp.port:
netloc += f':{urlcomp.port}'
curated_baseurl = '{}://{}{}'.format(urlcomp.scheme, netloc, urlcomp.path)
Copy link
Member

Choose a reason for hiding this comment

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

An fstring would be more readable I think.

@bsipocz
Copy link
Member

bsipocz commented Oct 1, 2024

Failures are upstream server and unrelated.

@bsipocz bsipocz merged commit 18b0f1a into astropy:main Oct 1, 2024
12 of 13 checks passed
bsipocz added a commit that referenced this pull request Oct 14, 2024
BUG: Include port number if present
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.

dal.vosi.EndpointMixin swallows port numbers when looking for endpoints
2 participants