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.

Sign up for GitHub

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

Jump to bottom

Adding a distribution section to the JEP JEP and small tweaks #38

Merged
merged 5 commits into from
Mar 7, 2019
Merged

Adding a distribution section to the JEP JEP and small tweaks #38

merged 5 commits into from
Mar 7, 2019

Conversation

choldgraf
Copy link
Contributor

@choldgraf choldgraf commented Mar 4, 2019 •
edited
Loading

Hey all - this PR builds off of #29 to add some language to the JEP enhancement proposal. There's a relevant issue to discuss this here: #27

This PR is primarily about adding a "distribution" section to the "meta JEP". It mentions using a public website as an "archive" of finished JEPs. I tried not to tie it in to any specific technology, since I feel like that is more of an implementation detail and we can cross that bridge when we come to it. Hope that's OK.

The PR also:

  • Cleans up a few formatting things I found in there, and adds links to the ipython/python *EP process
  • Adds this JEP to the list of active JEPs in the readme (I am a bit unclear that status this JEP has...so I just left what was in the metadata for this one)

Lemme know what folks think!

willingc
willingc reviewed Mar 5, 2019
View reviewed changes
Copy link
Member

@willingc willingc left a comment

Choose a reason for hiding this comment

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

@choldgraf Thanks for the PR. I've made some clarifying suggestions.

29-jep-process/README.md Outdated Show resolved Hide resolved
29-jep-process/README.md Outdated Show resolved Hide resolved
29-jep-process/README.md Outdated

### The JEP public archive website

A list of all current JEPs and the content present in the pull request exists at
Copy link
Member

Choose a reason for hiding this comment

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

The JEP itself should be self contained for relevant links to discussions. At a minimum the website should have a reader-friendly version of JEPs.

I would clarify that the JEP itself, not the website, contains this information. The JEPs can be displayed to provide the community easier navigation and access to the information contained in the JEP repo.

29-jep-process/README.md Outdated Show resolved Hide resolved
29-jep-process/README.md Outdated Show resolved Hide resolved
29-jep-process/README.md Outdated Show resolved Hide resolved
29-jep-process/README.md Outdated

- Final Comment Period (FCP): A finite-time, post-approval period for the community to make final comments.
- **Final Comment Period (FCP)**: A finite-time, post-approval period for the community to make final comments.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be pre-approval so that folks have a window of time to comment prior to JEP acceptance.

README.md
@@ -25,7 +25,7 @@ Below is a list of JEPs that have been submitted. To view the discussion around
| 0025 | Completed | [Enterprise Gateway](https://github.com/jupyter/enhancement-proposals/pull/25) |
| 0026 | **Submitted** | [Add Language Server Support to Jupyter Server and jupyterlab-monaco](https://github.com/jupyter/enhancement-proposals/pull/26) |
| 0028 | **Submitted** | [Standalone Jupyter Server](https://github.com/jupyter/enhancement-proposals/pull/28) |

| 0029 | **Submitted** | [Jupyter Enhancement Proposal updates](https://github.com/jupyter/enhancement-proposals/pull/29)
Copy link
Member

Choose a reason for hiding this comment

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

Excellent!

@captainsafia
Copy link
Member

Looks good! I'll ditto @willingc's feedback.

Could you update the PR description to mention the issue so that people reading the main issue can click off to get to the relevant PRs. Thanks!

willingc and others added 2 commits March 5, 2019 08:15
@choldgraf
Copy link
Contributor Author

quick update:

  • I added a sentence to linking to the issue at the top of this thread
  • Suggested edits from @willingc accepted
  • I changed "post-approval" to "pre-approval" since I agree this seems to make the most sense gauging from the flow of JEP states that @captainsafia described in the diagram.
  • I tried updating the "distribution" section to be more clear about how the website text relates to the JEP content. Does that work @willingc ?

willingc
willingc approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@willingc willingc left a comment

Choose a reason for hiding this comment

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

These updates look great to me @choldgraf. I'll leave for others to comment / merge.

@captainsafia
Copy link
Member

cc: @mckev-amazon

captainsafia
captainsafia approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Thanks!

mckev-amazon
mckev-amazon approved these changes Mar 5, 2019
View reviewed changes
Copy link

@mckev-amazon mckev-amazon left a comment

Choose a reason for hiding this comment

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

lgtm!

@parente parente mentioned this pull request Mar 6, 2019
Merged
@willingc willingc merged commit 7ddb06b into jupyter:master Mar 7, 2019
@willingc
Copy link
Member

willingc commented Mar 7, 2019

Thanks folks. 👍

@choldgraf
Copy link
Contributor Author

yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mckev-amazon mckev-amazon mckev-amazon approved these changes

@willingc willingc willingc approved these changes

@captainsafia captainsafia captainsafia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@choldgraf @captainsafia @willingc @mckev-amazon