Warn on DAL overflow

[andamian]

Fix for #322 .

[codecov]

Codecov Report

Merging #329 (740953d) into main (949693c) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #329      +/-   ##
==========================================
+ Coverage   75.49%   75.52%   +0.02%     
==========================================
  Files          44       44              
  Lines        5125     5131       +6     
==========================================
+ Hits         3869     3875       +6     
  Misses       1256     1256              

Impacted Files	Coverage Δ
pyvo/dal/__init__.py	100.00% <100.00%> (ø)
pyvo/dal/exceptions.py	77.02% <100.00%> (+0.63%)	⬆️
pyvo/dal/query.py	85.12% <100.00%> (+0.15%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[msdemlei]

Looks basically fine too me. A few nitpicks that I'd rather see fixed/addressed before merging this:

• Quite a few of the test VOTables were re-written and thus have meaningless changes. I'm not saying that's a drama, but saying a few words how these changes came about would be great.
• There's now an overflow warning in pyvo/dal/tests/test_datalink.py::test_datalink_batch, which is because the obscore test result (pyvo/dal/tests/data/datalink/datalink-obscore.xml) is overflowing on creation. I'd be fine with both manually removing the INFO element and with swallowing the warning. It shouldn't go unhandled, though.

[andamian]

Looks basically fine too me. A few nitpicks that I'd rather see fixed/addressed before merging this:

• Quite a few of the test VOTables were re-written and thus have meaningless changes. I'm not saying that's a drama, but saying a few words how these changes came about would be great.

I've run the script that generates these files with a different version of astropy(votable). Where should I add this comment?

• There's now an overflow warning in pyvo/dal/tests/test_datalink.py::test_datalink_batch, which is because the obscore test result (pyvo/dal/tests/data/datalink/datalink-obscore.xml) is overflowing on creation. I'd be fine with both manually removing the INFO element and with swallowing the warning. It shouldn't go unhandled, though.

Good point. I've missed that.

[andamian]

@msdemlei - I've opted for removing the OVERFLOW status from the test data since that was a byproduct of the way they were generated (a SELECT TOP would not have had those in).

[msdemlei]

On Mon, May 16, 2022 at 09:15:08AM -0700, Adrian wrote: > * Quite a few of the test VOTables were re-written and thus have > meaningless changes. I'm not saying that's a drama, but saying a > few words how these changes came about would be great. I've run the script that generates these files with a different version of `astropy`(`votable`). Where should I add this comment?

Well, under normal circumstances I'd use the commit message ("recreated some test VOTables in the process; changes are syntactic only"). Since the branch isn't merged yet, I guess a rebase -i editing the message would be an option. But as I said: I don't think anyone who should be reading this code would ever be confused by the changes even when they're not mentioned in the commit message.

[tomdonaldson]

This looks good to me. Just the one question about the placement of the INFO element in the test data.

[tomdonaldson]

This is interesting. I had to go back and look at the DALI standard to see that this would work to the OVERFLOW case (i.e., that OVERFLOW would come last).