SpiceyPy application to PlanetaryPy

[AndrewAnnex]

look at me getting things done early. This will need a coordinator to volunteer themselves.

[rbeyer]

Thank you for proposing this package as an affiliated package! I'm happy to confirm that your package is now under review and we'll post the results of the review here.

[rbeyer]

This package has been reviewed for inclusion in the PlanetaryPy affiliated package ecosystem by a member of the PlanetaryPy community as well as myself, and I have synthesized the results of the review here.

You can find out more about our review criteria in Reviewing affiliated packages. For each of the review categories below we have listed the score and have included some comments.

Functionality/Scope
No further comments
Integration with PlanetaryPy ecosystem
Since there is only one affiliated package currently in PlanetaryPy, this was simple to evaluate.
Documentation
The introduction paragraph on the GitHub repo should probably be replicated to start the readthedocs documentation. Without it, the readthedocs introduction pages don't properly lay out what the package is about. However, there is good API documentation and examples. Overall, documentation is well done.
Testing
Extensive tests and evidence of code coverage is present. The two paragraphs in the Contributing document are not terribly useful for a developer new to SpiceyPy, and further documentation about the potentially complex testing could be helpful.
Development status
No further comments
Python version compatibility
Tests are present for Python 3.6, 3.7, 3.8, and 3.9

Your package has a reasonable set of governance documents, but there is no indication of how someone might become a SpiceyPy maintainer, which you may want to consider adding.

Summary/Decision: Everything looks great, and we're happy to confirm that this package is accepted as an affiliated package! :trophy:

If you have any follow-up questions or disagree with any of the comments above, leave a comment and we can discuss it here. At any point in the future you can request a re-review of the package if you believe any of the scores should be updated - contact the coordination committee, and we’ll do a new review.

[AndrewAnnex]

okay for future reviews, I think it would critical for feedback to be written in a more constructive manner, possibly with the reviewer issuing issues to the target repository. Otherwise, comments should be communicated in private if they do not change the evaluation criteria of the review. Some of these suggestions are fair but trivial to address as time permits. I'll address some other issues I have with this review here:

Also the readthedocs citation information is outdated.

This is not true, it is very up to date as I added it a week or two ago, maybe this review was conducted before the recent addition to the docs? see https://spiceypy.readthedocs.io/en/main/citation.html?highlight=citing

The two paragraphs in the Contributing document are not terribly useful for a developer new to SpiceyPy

There are more than two paragraphs, which two paragraphs? Why are they not useful? What would be better? This feedback is not providing any meaningful details.

further documentation about the potentially complex testing could be helpful.

Again, what additional documentation is needed beyond what is already explained in the document?

[rbeyer]

okay for future reviews, I think it would critical for feedback to be written in a more constructive manner, possibly with the reviewer issuing issues to the target repository. Otherwise, comments should be communicated in private if they do not change the evaluation criteria of the review. Some of these suggestions are fair but trivial to address as time permits.

That's an interesting point. If you go to the extreme, all reviewer concerns could be written as Issues to the project repo. So does that mean that we never issue any comments here? Or only issue them for non-green items? Not sure, but we should definitely talk more about this. I'll add it as a discussion point for our February TC meeting.

I'll address some other issues I have with this review here:

Also the readthedocs citation information is outdated.

This is not true, it is very up to date as I added it a week or two ago, maybe this review was conducted before the recent addition to the docs? see https://spiceypy.readthedocs.io/en/main/citation.html?highlight=citing

You're absolutely right, I knew I shouldn't have tried to do this right before bed. I will amend the review.

The two paragraphs in the Contributing document are not terribly useful for a developer new to SpiceyPy

There are more than two paragraphs, which two paragraphs? Why are they not useful? What would be better? This feedback is not providing any meaningful details

further documentation about the potentially complex testing could be helpful.

Again, what additional documentation is needed beyond what is already explained in the document?

These comments are about Testing, since they're under the testing criteria, and the two paragraphs referred to are those about testing in the CONTRIBUTING document, sorry for not being more clear.

The reviewer and I felt that these paragraphs were not newbie-friendly. Details about SpiceyPy testing having to do with the comments about using spice.kclear() or what's going on with the SPICE kernels that are apparently getting downloaded are clearly complex, and are quite different concerns than what happens during the typical Python testing process. We would have liked to have seen a more detailed description about these aspects. I know both Python and SPICE, but I'd have to do a bunch of code-reading to understand what's going on with these things. They are mentioned, which is better than nothing, but the "theory" of when and how a developer would make the choices that these options provide is not detailed.

There is a completely valid perspective that this is sufficient, and that this is a completely valid bar for a developer to pass over to be an effective contributor. Not only must they understand Python testing, they must also understand the process that NAIF uses for testing cspice. Taking no action on this comment is completely valid.

[AndrewAnnex]

okay great, those comments very much clear my objections up and is the level of detail I think should be the expectation for all reviews.

As for the issue bit, if a problem noticed in a review is sufficiently problematic (non-green) then I think github issues make it easier to track the review across repositories, provided that the contributor/author/maintainers is/are interested in continuing with the review to keep the process as open and constructive.

For example in reviews like this, we would see links to the corresponding issues, and be able to see when different issues were resolved as issues/commits. Issues provide a space to address specific problems with the level of detail needed to address the issue in a organized manner.

Of course, not all issues in the review warrant creating issues posts, it should be up to the discretion of the reviewer's/manager of the review. If a issue is a non-blocker for the review, like those above, issues are still a good way to capture that feedback so long as they are communicated as non-blocker/non-critical suggestions.

This is how JOSS reviews work from my recollection, and when you go back to the public reviews the traceability is very nice to see.