More careful NULL value handling in tapregext data limits.

[msdemlei]

This ought to fix bug #658.

Actually, I considered including the (perhaps more relevant) default upload limit in the describe() output, too. I will if someone asks for it here...

[codecov]

Codecov Report

Attention: Patch coverage is 78.78788% with 7 lines in your changes missing coverage. Please review.

Project coverage is 83.08%. Comparing base (e7a8074) to head (8e909ee).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/io/vosi/tapregext.py	22.22%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
+ Coverage   82.76%   83.08%   +0.31%     
==========================================
  Files          75       75              
  Lines        7684     7715      +31     
==========================================
+ Hits         6360     6410      +50     
+ Misses       1324     1305      -19     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ManonMarchand]

I think default and hard limits are both interesting in the description.

[msdemlei]

On Thu, Mar 20, 2025 at 02:09:10AM -0700, Manon Marchand wrote: ManonMarchand left a comment (astropy/pyvo#659) I think default and hard limits are both interesting in the description.

Aw... On thinking again, I've re-read TAPRegExt to remind myself what default and hard actually means for uploads. You see, for the output, *default* has an operational meaning ("if you want more, you have to pass MAXREC"). For uploads, there is no analog to MAXREC; when I cooked up TAPRegExt I didn't want to invent yet another type and hence wrote this (https://ivoa.net/documents/TAPRegExt/20120827/REC-TAPRegExt-1.0.html#reslimits) to justify why output and upload limits share a type: The default value advises clients about the server's wishes as to a limit above which some sort of acknowledgement should be requested from the user. The hard limit gives the maximum size of an upload to the server. Since we don't do any advising (and I don't think we really ought to in pyVO), I'd say there is little point in displaying the default limit and current describe's output is just fine. So, I'd say this is good to go. Manon, can I charm you to do a review since you've already looked at it? I'd say we don't need a changelog item for this -- or do we? Thanks!

[ManonMarchand]

This explains why the two values are identical in the capabilities endpoints I know! I guess the services can add warnings in the response when people overshoot the default limit so we don't need it in pyVO. And it it very strange from IRSA not to set an upload limit at all.

The PR fixes a very rare issue. It can go without a changelog entry. Could you maybe add a test in https://github.com/astropy/pyvo/blob/main/pyvo/io/vosi/tests/test_capabilities.py ?

[msdemlei]

On Thu, Mar 20, 2025 at 06:27:17AM -0700, Manon Marchand wrote: The PR fixes a very rare issue. It can go without a changelog entry. Could you maybe add a test in https://github.com/astropy/pyvo/blob/main/pyvo/io/vosi/tests/test_capabilities.py ?

Fair enough, in particular because test coverage here is fairly spotty (there's no coverage of tapregext's describe, to begin with, but then the way these describe-s are done isn't very test-friendly). I've added a few tests that should fail when we assume too much about the content of TAPRegExt. We certainly could do better still, but I think I have covered a few spots that might blow up in the future.

[bsipocz]

Thank you so much for having this fixed so quickly!