Now passing strings to mimetypes.guess_extension. #553
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
msdemlei
force-pushed
the
fix-extension-derivation
branch
from
46bf53e to
95d36b2
Compare
msdemlei
force-pushed
the
fix-extension-derivation
branch
2 times, most recently
from
c0ab930 to
16bc54f
Compare
|
So, I think the remaining failure is unrelated (a fluke?). From my point of view, this is ready to go. |
andamian
reviewed
Jun 14, 2024
pyvo/dal/sia.py
Outdated
| @@ -903,7 +903,7 @@ def suggest_dataset_basename(self): | |||
| out = re.sub(r'\s+', '_', out.strip()) | |||
| return out | |||
|
|
|||
| def suggest_extension(self, *, default=None): | |||
| def suggest_extension(self, *, default='img'): | |||
There was a problem hiding this comment.
Just curious why img? I've noticed same assumption in suggest_dataset_basename.
|
On Fri, Jun 14, 2024 at 03:12:32PM -0700, Adrian wrote:
@andamian commented on this pull request.
> @@ -903,7 +903,7 @@ def suggest_dataset_basename(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
force-pushed
the
fix-extension-derivation
branch
from
16bc54f to
17ac11e
Compare
|
There is "dat" as a non-informative default extension elsewhere in pyvo, so that's what I'm going for now. |
|
(test failures were a glitch on a remote service) |
bsipocz
reviewed
Jun 27, 2024
pyvo/dal/adhoc.py
Outdated
| note the table row this DatalinkResults instance was generated | ||
| from. | ||
|
|
||
| This is mostly for pyVO-internal use. |
There was a problem hiding this comment.
given this, should this be prepended with an _?
|
OK, looking at the other PRs and discussions, and the commit history here, I think that the 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. |
Also changing the default extension to .dat (from None). Also, whatever was originally implementing mime2extension did not include the leading dot that mimetypes includes. We are doing away with that, too. This is supposed to address bug astropy#552.
bsipocz
force-pushed
the
fix-extension-derivation
branch
from
17ac11e to
3f3e89f
Compare
|
The backup commit is here: e2cdd91 |
bsipocz
approved these changes
Jun 27, 2024
|
Thank you @msdemlei! (I mark this for backport, in case we have 1.5.3 coming out sooner than 1.6) |
|
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.
|
bsipocz
added a commit
that referenced
this pull request
Oct 14, 2024
Now passing strings to mimetypes.guess_extension.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also changing the default for images to .img (from None).
This is supposed to address bug #552.
Fixes #552