search

astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
74 stars 50 forks source link

Now passing strings to mimetypes.guess_extension. #553

Closed msdemlei closed 2 weeks ago

msdemlei commented 1 month ago

Also changing the default for images to .img (from None).

This is supposed to address bug #552.

Fixes #552

msdemlei commented 4 weeks ago

So, I think the remaining failure is unrelated (a fluke?). From my point of view, this is ready to go.

msdemlei commented 3 weeks ago

On Fri, Jun 14, 2024 at 03:12:32PM -0700, Adrian wrote:

@andamian commented on this pull request.

@@ -903,7 +903,7 @@ def suggest_datasetbasename(self): out = re.sub(r'\s+', '', out.strip()) return out

  • def suggest_extension(self, *, default=None):
  • def suggest_extension(self, *, default='img'):

Just curious why img? I've noticed same assumption in suggest_dataset_basename.

I'm not particularly married to "img", and now that you mention it, since this is not only for SIAP (and hence images), I notice it may if fact not be particularly appropriate. I certainly have no strong preferences, except the None we have now seems a choice that will confuse our users more than just about anything else. What about "dat"? "bin"? "octets"? "data"?

msdemlei commented 3 weeks ago

There is "dat" as a non-informative default extension elsewhere in pyvo, so that's what I'm going for now.

msdemlei commented 3 weeks ago

(test failures were a glitch on a remote service)

bsipocz commented 2 weeks ago

OK, looking at the other PRs and discussions, and the commit history here, I think that the original_row related changes are added here by mistake in the very last commit iteration.

So, I'll make a backup of the current branch, but then will go ahead and remove those changes from this PR and merge it as the rest are fully finished with test and addressed review.

bsipocz commented 2 weeks ago

The backup commit is here: https://github.com/astropy/pyvo/commit/e2cdd91942f2516590fbd11c8f649cca6fb5b094

bsipocz commented 2 weeks ago

Thank you @msdemlei!

(I mark this for backport, in case we have 1.5.3 coming out sooner than 1.6)

msdemlei commented 2 weeks ago

On Thu, Jun 27, 2024 at 01:59:33PM -0700, Brigitta Sipőcz wrote:

@bsipocz commented on this pull request.

Can we have a test for original_row, and its setter method? As far as I see it doesn't come up with any of the tests.

Sorry that that code appeared in this PR in the first place -- I was juggling a few too many branches at that moment. In the actual PR that this stuff belongs to, #559, the setter method is (fortunately) gone, and there are some basic tests for the main code paths involving original_row, too.