API: changing optional arguments to be keyword only

[bsipocz]

This PR is to change optional arguments to be keyword only with the intention of making the API a bit cleaner and stricter over time.

This currently causes a lot of internal test failures (commit that fixes those is to come), but I wanted to get this out of the door asap so people can comment on whether this is a direction we should go.

Closes #408

[tomdonaldson]

I like these call signatures as they will make the method calls much more clear and explicit. However isn't this a rather significant API change? Like in the test failures, won't existing user code fail without the keyword specifications?

[bsipocz]

@tomdonaldson - I don't see any failures in astroquery, but will run the navo notebooks, too. And yes, it's a significant API change, but thus I would like to get them in sooner rather than later (assuming we decide we would want this).

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c4f2fa) 80.28% compared to head (9fbcc6b) 80.28%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #507   +/-   ##
=======================================
  Coverage   80.28%   80.28%           
=======================================
  Files          52       52           
  Lines        6152     6152           
=======================================
  Hits         4939     4939           
  Misses       1213     1213           

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

[bsipocz]

@tomdonaldson - I don't see any errors due to this in the navo repo either (the is one timeout, but it should be unrelated, and hopefully there isn't anything lurking in the rest of that notebook)

[msdemlei]

On Fri, Dec 15, 2023 at 02:57:29PM -0800, Brigitta Sipőcz wrote: @tomdonaldson - I don't see any errors due to this in the navo repo either (the is one timeout, but it should be unrelated, and hopefully there isn't anything lurking in the rest of that notebook)

Hmyeah, but I'm sure there's a lot of user code that will break. Now, I agree that it's probably worthwhile to purse this API cleanup, and we should certainly merge the fixes to the tests and to documentation so people don't copy bad practices. As to merging the signature changes... ah well, if there was a way to sanely just raise deprecation warnings for two or three minor versions, I'd say that'd be fine. But I don't think there is, and so perhaps we should briefly stop to think whether that's an indication we ought to do a major version change? And if so, what else we would change? For instance, there's the currently very cavalier handling of XML namespaces that probably can only be fixed by slightly breaking the API, and perhaps we would be a little less property-happy...? I'm sure there are many other places that could be improved (datalink processing services come to mind). Disclaimer: I'm afraid I can't pledge *significant* resources for such an overhaul, but I thought when we touch a large part of the API, perhaps now is the time to think about it.

[bsipocz]

So @msdemlei, would you say hold this up and release v1.5 without this PR?

As you see, nothing in fact needed to be changed in our own documentation, nothing got caught up in astroquery, nor in the navo notebooks, so while this could indeed cause failures, in practice I would hope that people are sanely use the API rather than feeding it a lot of positional arguments.

(I'm also very much in favour of cleaning up or simplifying the API, but I also don't see that I will be able to assign significant resources for it. If I run into something bothersome, I'll try to clean it, but it all will be incremental).

[msdemlei]

On Mon, Dec 18, 2023 at 10:49:28PM -0800, Brigitta Sipőcz wrote: So @msdemlei, would you say hold this up and release v1.5 without this PR?

My instinct (after having broken too many things on the assumption "Aw, nobody will have done that") would be to release 1.5 and then merge the signature changes early in 1.6; I'd hope we'll get a bit more experience with the ramifications out there in the wild in the 1.6 cycle.

As you see, nothing in fact needed to be changed in our own documentation, nothing got caught up in astroquery, nor in the navo notebooks, so while this could indeed cause failures, in practice I

Sure, but these are all rather controlled and civilised environments. Out there, it's folks who will type just about anything to get the job done... As long as our documentation and tests show the right patterns, I'd say we've done enough for 1.5, and if we've had the whole run-up to 1.6 for people to notice that their code will break, at least we can then say "you could have spoken up earlier".

[bsipocz]

As long as our documentation and tests show the right patterns, I'd
say we've done enough for 1.5, and if we've had the whole run-up to
1.6 for people to notice that their code will break, at least we can
then say "you could have spoken up earlier".

OK. I'll run a through this to see whether any of these functions are brand new in 1.5, and if yes, than pull the updates for those, but leave the rest, that actually changes existing API alone. And then do the release.

[bsipocz]

I haven't noticed anything that could go in the release without changing the released API (compared to 1.4.2), so I'll separate out the test updates and merge that one, but leave the rest alone.

[bsipocz]

OK, so now that we're back in the dev cycle for 1.6, maybe this can go in as the first PR, so downstream will have all the time they can have to adjust 😄

[msdemlei]

On Wed, Dec 20, 2023 at 11:12:06AM -0800, Brigitta Sipőcz wrote: OK, so now that we're back in the dev cycle for 1.6, maybe this can go in as the first PR, so downstream will have all the time they can have to adjust 😄

Fine with me.

[tomdonaldson]

I agree that this is ready to go, and that it makes sense to get it out early in the v1.6 dev cycle. Thanks @bsipocz for pushing improvements to the API.

After the holidays, let's announce the change on IVOA channels and encourage testing with the dev version.