Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sunkit-instruments affiliated package #335

Closed
7 tasks done
nabobalis opened this issue Sep 7, 2022 · 13 comments · Fixed by #352
Closed
7 tasks done

sunkit-instruments affiliated package #335

nabobalis opened this issue Sep 7, 2022 · 13 comments · Fixed by #352
Assignees
Labels
Affiliated Package Review An issue submitting a new Affiliated Package for review

Comments

@nabobalis
Copy link
Contributor

nabobalis commented Sep 7, 2022

Package Details

Description of Package

Stores instrument code that does not belong to a package that is maintained by the instrument teams.

Package Review

Editor Submission Checklist

  • Is the submission appropriate (compatible with the SunPy CoC)?
  • Does the project have an appropriate license?
  • Is the project in an online version control system?
  • Does the project provide a Python interface?
  • Is the project on PyPI?
  • Is the project useful to the solar physics community?
  • Version that was reviewed (tag or commit hash)

Instructions to Reviewer

Please copy the following and select the ranking for each criteria, the full review criteria can be found here:

* Functionality           : General Package / Specialized Package / Not Relevant
* Integration             : Well integrated / Partially Integrated / Minimal Integration
* Documentation           : Extensive / Some / Little
* Testing                 : Excellent / Good / Needs Work
* Duplication             : None / Some / Major
* Community               : Excellent / Good / Needs Work
* Development Status      : Stable / Subject to Change / Low Activity / Needs Work
@nabobalis nabobalis added the Affiliated Package Review An issue submitting a new Affiliated Package for review label Sep 7, 2022
@Cadair
Copy link
Member

Cadair commented Sep 7, 2022

@dstansby You up for doing this review?

@dstansby
Copy link
Member

dstansby commented Sep 7, 2022

👍

@dstansby
Copy link
Member

dstansby commented Sep 9, 2022

  • Functionality : General Package
  • Integration : Partially Integrated
  • Documentation : Good
  • Testing : Excellent
  • Duplication : None
  • Community : Needs Work
  • Development Status : Low Activity

Notes

Integration

I've opened a bunch of issues on the repostory. Most of the integration improvements that could be made are returning astropy tables from functions where they currently return dicts. There's also a few areas where a standard time object or unit object should be accepted as input/output.

Docs

API docs are good (with some small gaps here or there). There is only a single example for the whole library. There are no guides or tutorials for each sub-package.

Community

I cannot find a code of conduct anywhere obvious, either in the repository or linked in the documentation. Otherwise all the other community criteria are met.

Any questions about my review let me know - if any of the above points are improved, I would be very happy to re-review.

@Cadair
Copy link
Member

Cadair commented Sep 12, 2022

I cannot find a code of conduct anywhere obvious

I don't know how we make the fact clear that all sunpy maintained packages follow the sunpy CoC. The CoC is linked from the sidebar in the GH repo (as it inherits from the org).

@dstansby
Copy link
Member

dstansby commented Sep 12, 2022

I've just found that, but given I was actively looking for a CoC and couldn't find one I think it needs to be more obvious. Places I looked were:

  • A file in the repository (code_of_conduct.rst or similar)
  • README.md
  • The documentaiton landing page

@Cadair
Copy link
Member

Cadair commented Sep 12, 2022

👍 This probably applies to more than just sunkit-instruments.

The file in the repo is a big 👎 from me, we do not want to be maintaining more than one copy of the CoC text. The canonical version is here: https://github.com/sunpy/.github/blob/main/CODE_OF_CONDUCT.md which is built into the website here: https://sunpy.org/coc we should link to that in the readme and docs for all packages of ours.

@Cadair
Copy link
Member

Cadair commented Sep 12, 2022

Actually I am also 👎 on just throwing it in the docs given we already have it in the nav bar. We should probably add it to the theme in the footer as we have discussed elsewhere.

@Cadair
Copy link
Member

Cadair commented Sep 12, 2022

I don't think we have really documented how exactly we handle responses to reviews, but given sunpy/sunkit-instruments#78 and the fact we do actually have a CoC, will you re-review?

@wtbarnes
Copy link
Member

@dstansby Given the responses above, are you willing to re-review or does your current review still stand?

@dstansby
Copy link
Member

I forgot to put the verson reviewed in my original review, but I reviewed v0.3.0. It seems like there hasn't been an updated release with any changes, but if a new release is made I would be happy to re-review.

@Cadair
Copy link
Member

Cadair commented Oct 12, 2022

v0.3.1 is now released if you could re-review that.

@dstansby
Copy link
Member

  • Functionality : General Package
  • Integration : Partially Integrated
  • Documentation : Good
  • Testing : Excellent
  • Duplication : None
  • Community : Good
  • Development Status : Low Activity

Version reviewed: v0.3.1

@wtbarnes
Copy link
Member

Given the scores, sunkit-instruments has passed the review and can now be added to the webpage as an affiliated package.

nabobalis pushed a commit that referenced this issue Jan 11, 2023
Co-authored-by: Nabil Freij <nabil.freij@gmail.com>
Fixes #335
Fixes #331
Fixes #322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affiliated Package Review An issue submitting a new Affiliated Package for review
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants