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

MAINT: Corrections to some of the parameter types in the docs and a couple lint-related fixes #653

Merged
merged 2 commits into from
Feb 25, 2025

Conversation

stvoutsin
Copy link
Contributor

Summary

This PR mostly is just to cleanup some of the documentation and specifically the parameter types. as well as a couple linting issues like classes missing from __all__ declaration, unneeded parenthesis and changing camel-case to snake-case in one instance.

Assuming these changes make sense, am I right to assume this doesn't need a changelog entry?

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.47%. Comparing base (338fc8f) to head (2e4f2dd).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #653   +/-   ##
=======================================
  Coverage   82.47%   82.47%           
=======================================
  Files          72       72           
  Lines        7480     7480           
=======================================
  Hits         6169     6169           
  Misses       1311     1311           

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

@stvoutsin stvoutsin marked this pull request as ready for review February 25, 2025 20:22
pyvo/dal/vosi.py Outdated
@@ -14,10 +14,10 @@
from ..utils.decorators import stream_decode_content, response_decode_content
from ..utils.http import use_session

__all__ = ['CapabilityMixin', 'VOSITables']
__all__ = ['AvailabilityMixin', 'CapabilityMixin', 'VOSITables']
Copy link
Member

Choose a reason for hiding this comment

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

AvailabilityMixin is deprecated, please don't put it back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I missed that it was deprecated, I've removed it again

@bsipocz bsipocz added this to the v1.6.2 milestone Feb 25, 2025
Copy link
Contributor

@andamian andamian left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you

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.

OK, now it looks OK.

Btw, please don't add classes to __all__ just for lining purposes as we may omit some as non public API.

dal/query is not included in the API docs, so adding UploadList doesn't have an effect, yet, it should not be added just to make the linter happy.

As for the docstrings, thanks for fixing those. Adding a numpydoc linter to CI is on my list, so any cleanup is welcome for now.

@bsipocz bsipocz merged commit c2f7a09 into astropy:main Feb 25, 2025
13 checks passed
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.

3 participants