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: deprecate AvailabilityMixin #413

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

bsipocz
Copy link
Member

@bsipocz bsipocz commented Jan 27, 2023

This is to close #214

The class wasn't exposed to the user documentation so it's borderline whether this needs a deprecation and a changelog, but it was part of the modules's __all__, so I treat it as it was part of the public API (as #411 revealed that a few API docs are mistakenly missing from the rendered documentation)

[edit]: it was in fact mentioned in the narrative docs, it was only the API docs that wasn't exposed. So I've now removed that narrative, too.

@bsipocz bsipocz added this to the v1.5 milestone Jan 27, 2023
@bsipocz bsipocz requested a review from msdemlei January 27, 2023 04:57
@bsipocz bsipocz force-pushed the MAINT_deprecate_AvailabilityMixin branch from 700628a to bdc71cc Compare January 27, 2023 05:38
@codecov
Copy link

codecov bot commented Jan 27, 2023

Codecov Report

Merging #413 (c32f563) into main (eb24f44) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #413      +/-   ##
==========================================
+ Coverage   79.92%   79.93%   +0.01%     
==========================================
  Files          52       52              
  Lines        6011     6016       +5     
==========================================
+ Hits         4804     4809       +5     
  Misses       1207     1207              
Impacted Files Coverage Δ
pyvo/dal/vosi.py 69.64% <100.00%> (+1.41%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bsipocz bsipocz force-pushed the MAINT_deprecate_AvailabilityMixin branch from bdc71cc to ccc6e8d Compare January 27, 2023 05:51
Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

Excellent, thanks a lot.

(and: made a note to configure my vi to swallow trailing blanks, although the ones in this diff by and large weren't mine).

@bsipocz
Copy link
Member Author

bsipocz commented Jan 27, 2023

While sleeping on it I realised that this won't be sufficient as is, users won't come across this deprecation warning as they are not directly using the base class, but they face the missing properties as I removed subclassing this for the services.

cc @plim in case she can recall of a precent of deprecating a base class, so I can follow an already used recipe.

Otherwise I'll plan to add back the subclassing during the deprecation period and deprecate the individual inherited parts hoping not to pepper the users with unnecessary warnings, but also make them a chance to see those.

ps: I'm sorry about the trailing whitespaces, I haven't noticed that my editor touched on those when I made the commit, it wasn't my intention to do any such cleanups here.

@bsipocz bsipocz marked this pull request as draft January 27, 2023 16:35
@bsipocz bsipocz force-pushed the MAINT_deprecate_AvailabilityMixin branch from ccc6e8d to 1414ba4 Compare February 23, 2023 02:35
@bsipocz bsipocz force-pushed the MAINT_deprecate_AvailabilityMixin branch from 1414ba4 to c32f563 Compare April 20, 2023 01:23
@bsipocz bsipocz marked this pull request as ready for review April 20, 2023 01:23
@bsipocz bsipocz requested a review from msdemlei April 20, 2023 01:27
@bsipocz
Copy link
Member Author

bsipocz commented Apr 20, 2023

This should now be done properly, e.g. keeping the properties inherited from the AvailabiltyMixin base class deprecated rather than being removed straight away.

Copy link
Contributor

@msdemlei msdemlei left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this!

I've briefly considered whether I'd do a trailing-whitespace-eating commit first to make this diff a bit tidier, but interestingly this is about the last file with trailing blanks in docs (and in the source code, there's just a few XML files left), so I think it's ok to let that cleanup in with this otherwise harmless commit.

@bsipocz bsipocz merged commit 915b496 into astropy:main Apr 20, 2023
@bsipocz bsipocz deleted the MAINT_deprecate_AvailabilityMixin branch April 17, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dal.vosi: Drop AvailabilityMixin
2 participants