Skip to content

IO registry write returns results - #11916

Merged
mhvk merged 2 commits into
astropy:mainfrom
nstarman:io_return-in-write
Jul 2, 2021
Merged

IO registry write returns results#11916
mhvk merged 2 commits into
astropy:mainfrom
nstarman:io_return-in-write

Conversation

@nstarman

@nstarman nstarman commented Jul 2, 2021 •
edited by mhvk
Loading

Copy link
Copy Markdown
Member

This shouldn't change anything for existing writers, but allows for registered write methods to return something, if they so choose.

Signed-off-by: Nathaniel Starkman (@nstarman) [email protected]

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will
review this pull request of some common things to look for. This list
is not exhaustive.

  • Do the proposed changes actually accomplish desired goals?
  • Do the proposed changes follow the Astropy coding guidelines?
  • Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
  • Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
  • Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see "When to rebase and squash commits".
  • Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the Extra CI label.
  • Is a change log needed? If yes, did the change log check pass? If no, add the no-changelog-entry-needed label.
  • Is a milestone set? Milestone must be set but astropy-bot check might be missing; do not let the green checkmark fool you.
  • If the milestone set requires a backport to release branch(es), apply the appropriate backport-X.Y.x label(s) before merge.

@nstarman
nstarman force-pushed the io_return-in-write branch from 9ce89ca to e97ac86 Compare July 2, 2021 03:10
@mhvk mhvk added this to the v5.0 milestone Jul 2, 2021
mhvk
mhvk reviewed Jul 2, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes a lot of sense to me to allow writers to return something (almost always None anyway), though I wonder whether there should not be a minimal test (and docs?), even though obviously you will use it in future PRs... Maybe one case where splitting it off is not the right way? Ping @taldcroft

@taldcroft

taldcroft commented Jul 2, 2021

Copy link
Copy Markdown
Member

My question is just about consistency and the possibility that different writers return different content results. What's the planned use case?

@nstarman

nstarman commented Jul 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

I wonder whether there should not be a minimal test (and docs?)

Sure thing. I'll slap a changelog on there too.

My question is just about consistency and the possibility that different writers return different content results.

I guess that's a possibility. But can't readers return different content results? Obviously that would be funny if a Table reader returned a SkyCoord... but it could.

What's the planned use case?

For me, the object transformations I discuss in the Cosmology IO PR: Cosmology.from_(Planck18.to_("table")) == Planck18.

But more generally, writers might return memory addresses, status reports, etc.

@taldcroft

taldcroft commented Jul 2, 2021

Copy link
Copy Markdown
Member

would be funny if a Table reader returned a SkyCoord... but it could.

In general a unified I/O reader needs to return a subclass of the calling class.

@taldcroft

taldcroft commented Jul 2, 2021

Copy link
Copy Markdown
Member

Anyway, this idea seems fine.

@nstarman
IO registry write returns results
20c3509 
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
@nstarman
nstarman force-pushed the io_return-in-write branch from e97ac86 to 20c3509 Compare July 2, 2021 15:52
@nstarman
nstarman requested a review from mhvk July 2, 2021 15:52
mhvk
mhvk reviewed Jul 2, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks all OK except I think it would be nice to make the changelog entry a little more descriptive. (Alternatively, I'm also fine to not have a changelog entry, since this is no API change from the user perspective for anything currently in astropy!)

Comment thread docs/changes/io.registry/11916.api.rst Outdated
@nstarman @mhvk
changelog
666d1f2 
Co-authored-by: Marten van Kerkwijk <[email protected]>
Signed-off-by: Nathaniel Starkman (@nstarman) <[email protected]>
mhvk
mhvk approved these changes Jul 2, 2021
View reviewed changes

@mhvk mhvk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mhvk mhvk mentioned this pull request Jul 2, 2021
Merged
9 tasks done
@mhvk

mhvk commented Jul 2, 2021

Copy link
Copy Markdown
Contributor

Failing check is unrelated (#11918). Since @taldcroft agreed with the idea, let's get this in!

@mhvk
mhvk merged commit 30a50d2 into astropy:main Jul 2, 2021
@nstarman
nstarman deleted the io_return-in-write branch July 2, 2021 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhvk mhvk mhvk approved these changes

Assignees

No one assigned

Labels

io.registry Refactoring

Projects

Cosmology, the Expansion
Status: Done

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

3 participants

@nstarman @taldcroft @mhvk