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

CEP-001: Definition of the proposal process #2256

Merged
merged 20 commits into from Mar 6, 2023
Merged

Conversation

Tobychev
Copy link
Contributor

This is a first draft of the "root" CEP that defines the purpose, scope and process for further proposals. Basically I took the gammapy and Astropy documents outlining their change proposal process and mashed them together, trying to abbreviate and restructure as seemed reasonable.

This is very much a draft, both in terms of the actual process proposed and because I don't know the markup magic so some things that are supposed to be links are broken.

The hope is that it will nevertheless sever as a useful basis for discussion.

@maxnoe maxnoe changed the title A start on the Ctapipe enhancment proposal process A start on the ctapipe enhancement proposal process Feb 15, 2023
@maxnoe maxnoe changed the title A start on the ctapipe enhancement proposal process CEP-001: Definition of the proposal process Feb 15, 2023
Tobychev and others added 7 commits February 15, 2023 13:47
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
Co-authored-by: Maximilian Linhoff <maximilian.linhoff@tu-dortmund.de>
@kosack
Copy link
Contributor

kosack commented Feb 17, 2023

For the documentation, I suggest we separate CEPs by accepted/rejected, at least in the index of all CEPs, maybe even better in the CEP folder structure itself (refile them as necessary). Reading some of the others like PIGs, I found it confusing to know if each was accepted or not without opening each one and reading.

Also, the version where the CEP was implemented (if applicable) would also be useful, so perhaps a table like

CEP Title Status Implemented in
00X the title here ACCEPTED N/A
00Y another title here ACCEPTED v0.19.1
00Z a bad idea REJECTED
00N the title here ACCEPTED

@Tobychev
Copy link
Contributor Author

For the documentation, I suggest we separate CEPs by accepted/rejected, at least in the index of all CEPs, maybe even better in the CEP folder structure itself (refile them as necessary). Reading some of the others like PIGs, I found it confusing to know if each was accepted or not without opening each one and reading.

I think the numpy solution of putting them in sections after status is enough, and likewise just updating the title of a CEP to include the version something was implemented is probably sufficient.

I pushed a change to where things got moved to subdirectories implementing this.

@maxnoe
Copy link
Member

maxnoe commented Feb 20, 2023

Why do we need a directory for drafts? I think we only merge a CEP once it is proposed? Or even only when it is actually accepted or rejected?

So we would only need accepted and rejected directories?

fixed typo.
kosack
kosack previously approved these changes Feb 22, 2023
Copy link
Contributor

@kosack kosack left a comment

Choose a reason for hiding this comment

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

Looks good. It's true that the Drafts folder may be superfluous, but it depends a bit on the review process (if its fully in PRs only or decoupled)

@Tobychev
Copy link
Contributor Author

@maxnoe Well the draft directory is simply where I put the draft as I work on them. It just seems sensible to keep drafts close to where they will end up once they are far along that they can be proposed, so no real harm in having it there to encourage others to keep things neat.

If we really get problems with people committing lots of drafts by mistake then we could just add it to a .gitignore.

@maxnoe
Copy link
Member

maxnoe commented Feb 22, 2023

Fine with me in theory, but in practice it creates problems now....

docs build failes due to:

/home/runner/work/ctapipe/ctapipe/docs/development/ceps/index.rst:16: WARNING: toctree glob pattern 'accepted/*' didn't match any documents
/home/runner/work/ctapipe/ctapipe/docs/development/ceps/index.rst:33: WARNING: toctree glob pattern 'drafts/*' didn't match any documents

Is there a sphinx option to not error for empty globs?

@Tobychev
Copy link
Contributor Author

Added a "Rejected" section to the index, removed the explicit section for drafts but left the folder in the filesystem. Not sure I care so much either way honestly...

More annoyingly is that I can't find a way to disable the warning-turned-error I get from trying to pre-emptively adding a glob for accepted CEPs...

kosack
kosack previously approved these changes Feb 23, 2023
@Tobychev
Copy link
Contributor Author

I just comment out the globbing for the two failing directory, not very elegant but it sort of works for now?

@maxnoe
Copy link
Member

maxnoe commented Feb 23, 2023

I just comment out the globbing for the two failing directory, not very elegant but it sort of works for now?

I think there is nothing really we can do about it, so I would be fine.

@kosack Since we both reviewed and approved here, shall we directly add this to the "accepted" ceps?

kosack
kosack previously approved these changes Mar 1, 2023
@maxnoe
Copy link
Member

maxnoe commented Mar 1, 2023

Build is failing now due to the move, need to adapt which globs are commented out:

/home/runner/work/ctapipe/ctapipe/docs/development/ceps/index.rst:25: WARNING: toctree glob pattern 'proposed/*' didn't match any documents
looking for now-outdated files... none found
pickling environment... done
/home/runner/work/ctapipe/ctapipe/docs/development/ceps/accepted/cep-001.rst: WARNING: document isn't included in any toctree

really annoying...

@kosack kosack merged commit 5f2d159 into cta-observatory:main Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants