-
-
Notifications
You must be signed in to change notification settings - Fork 38
Update APE process to have PRs be only for editorial review #115
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
Conversation
I think it should be a role to be consistent with our GH access policy. We already have Affiliated Editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Here are some comments.
https://github.com/astropy/astropy-APEs?tab=readme-ov-file#accepted-apes section needs to be renamed, and a new column with actual status (discussion, accepted, implemented, rejected, withdrawn, abandoned) should be a new column on the table. (Addressed)
Should Zenodo DOI be generated on merge, or wait till "accepted" stage? (Answered)
|
@pllim - I've tried to address your comments. For Zenodo, I don't have a strong feeling. Our current guidelines are to not even put final rejected APEs on Zenodo, so maybe we can just keep it as-is and just put finalized accepted APEs on there. |
pllim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! A bit more comments on second read.
Also maybe we can set this PR to just close #98 as that issue won't be that relevant with this new process in place.
|
Another big-picture question for @astrofrog : would you be open to adding some sort of timeline to the process? A discussion at the coordination meeting 2025 emphasized how one of the other concerns about the APE process is that sometimes comments trickle in over months which leads to some wasted time by the authors and general frustration. Of course it could be a separate APE but I think it might be natural to do with this since they're connected in process. |
|
Items for this PR from CoCo tag-up on 2025-07-25:
|
This comment was marked as resolved.
This comment was marked as resolved.
|
I took over and added a commit to address #115 (comment) . @eteq , does this look good now? |
|
Sorry for all the commits after approvals. I did a final read through and found some really outdated info, etc. Hopefully my new changes are not controversial. |
|
I find a lot of duplicated and possibly conflicting info between APE 1 and README. I think README should just point to APE 1. However, Zenodo instructions should stay in README because we cannot control upstream Zenodo changes and we do not want to have to update APE 1 every time Zenodo feels like changing things up. |
…orial changes only, and then once merged the APE is discussed on astropy-dev WIP
and fix typos
Co-authored-by: Erik Tollerud <[email protected]>
if applicable. This is to reduce duplicate and possibly conflicting info.
and make quotes consistent
pllim
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes from me as co-author below...
p.s. I thought single backticks would render but it is not being rendered properly in PR preview. I wonder if it would still render by GH properly after merge. Any knows? Should I always use double backticks or something else?
| ----------------------- | ||
|
|
||
| author: Perry Greenfield | ||
| author: Perry Greenfield, Lia Corrales, Thomas Robitaille, Erik Tollerud, Pey Lian Lim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the authors as laid out at the bottom of this very APE.
| We intend APEs to be the primary mechanisms for proposing major new features, | ||
| for collecting community input on an issue, and for documenting the design | ||
| decisions that have gone into Python. The APE author is responsible for | ||
| decisions that have gone into Astropy. The APE author is responsible for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this is a typo in the original text.
| building consensus within the community and documenting dissenting opinions. | ||
|
|
||
| Because the APEs are maintained as text files in a versioned repository | ||
| (indirectly since this wiki is versioned within github), their revision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the wiki is referring to. The APEs are directly version controlled in Git.
| There are three kinds of APE: | ||
|
|
||
| * A "Standard Track" APE describes a new feature or implementation for | ||
| * "Standard Track": A "Standard Track" APE describes a new feature or implementation for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making the style consistent with APE status section.
| ------------------ | ||
|
|
||
| The coordinating committee thought it was a honking great idea. | ||
| The Coordinating Committee thought it was a honking great idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it is "Coordination" now but perhaps it was "Coordinating" back then, so I opted not to fix the spelling.
| -------------------------- | ||
|
|
||
| Any pull requests or development branches containing work on this APE should be | ||
| Any pull requests or development branches containing work on this APE before acceptance should be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to clarify that any PRs actually implementing the APE after acceptance should go into Implementation section. Is this too confusing? Did I confuse myself? Please let me know if this is wrong.
| ------------------ | ||
|
|
||
| <To be filled in by the coordinating committee when the APE is accepted or rejected> | ||
| <To be filled in when status changes, if applicable, as laid out in APE 1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more complicated now, so just refer to APE 1 rather than repeating everything here.
| long-standing PEP (Python Enhancement Proposal) process, but generally not quite as | ||
| formally. Please consult `APE Purpose and Process`_ for full details. | ||
|
|
||
| APEs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the repo now can have APEs that are not yet accepted, we need new Status column.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the contents of the process here are moved to APE 1, where possible, to avoid duplication, and to reduce the risk of conflicting information.
But I left the Zenodo instructions here so we don't have to modify APE 1 again if Zenodo decides to change things up upstream.
mhvk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few nits, but looks good!
|
@ceb8 verbally re-approved. I am just going to merge in a few hours unless I hear objections, since we have multiple approvals and 2 weeks have elapsed. |
The current APE process causes pull requests to be open for very long and makes it difficult for someone to follow the discussion. I am suggesting here using a process more similar to the PEP process, whereby PRs to add APEs undergo an editorial review only for clarity, spelling, etc. Once the APE is merged, it can be discussed on astropy-dev, and then updated as many times as needed via PRs.
Accepted APEs can still be modified as is already outlined in APE 1, and for that the PR has to be finalized and accepted by the CoCo before it is merged, so it's a little different, but I think that's ok because modifications to accepted APEs should not be too substantial, otherwise they should be a new APE.
As written curerntly, this would require creating a GitHub team called @astropy/ape-editor-team - I don't think this necessarily needs to be a role (although it could be).
This is a draft for now, and we should of course discuss the different aspects here - I just wanted to put something to start.
After merge
@astropy/ape-editor-team