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

8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes #303

Closed

Conversation

kevinrushforth
Copy link
Member

@kevinrushforth kevinrushforth commented Sep 12, 2020

This PR updates the CONTRIBUTING.md guide to address the following:

  1. Clarify the process for adding new features / API changes, specifically that they must be discussed on the mailing list as the first step.
  2. Add a link to the mailing list in the section regarding contributing bug fixes.
  3. Remove the text about cross-linking the PR and JBS issue, and add a note that the Skara tooling takes care of this
  4. Remove the section about manually resolving the JBS issue, and add a note that the Skara bot automatically does this when the PR is integrated.
  5. Suggest the use of the "/reviewers 2" and "/csr" commands when appropriate
  6. Update the note regarding which JDK(s) to use.

Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/303/head:pull/303
$ git checkout pull/303

Update a local copy of the PR:
$ git checkout pull/303
$ git pull https://git.openjdk.java.net/jfx pull/303/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 303

View PR using the GUI difftool:
$ git pr show -t 303

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/303.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 12, 2020

👋 Welcome back kcr! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@kevinrushforth kevinrushforth self-assigned this Sep 12, 2020
Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

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

Additional comments:

  1. "Use Unix-style (LF) line endings not DOS-style (CRLF)" needs a comma before "not".
  2. "Line width is no more than 120 characters" I remember that it was 130 or 135 somewhere.
  3. "Wildcard imports (import foo.bar.baz.*) are forbidden" Junit imports use them extensively.
  4. ./gradlew all test will cause failure on webkit tests if it was not built.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
@kevinrushforth kevinrushforth marked this pull request as ready for review September 14, 2020 21:12
@openjdk openjdk bot added the rfr Ready for review label Sep 14, 2020
@mlbridge
Copy link

mlbridge bot commented Sep 14, 2020

@kevinrushforth
Copy link
Member Author

Regarding your additional comments:

  • "Use Unix-style (LF) line endings not DOS-style (CRLF)" needs a comma before "not".

Fixed.

  • "Line width is no more than 120 characters" I remember that it was 130 or 135 somewhere.

You're probably remembering an old version, but it's been 120 for a while now.

  • "Wildcard imports (import foo.bar.baz.*) are forbidden" Junit imports use them extensively.

Fixed to add an exception for wildcard static imports in tests.

  • ./gradlew all test will cause failure on webkit tests if it was not built.

Added a note about this and a pointer to the Web Testing doc.

@kevinrushforth
Copy link
Member Author

/reviewers 2

@openjdk
Copy link

openjdk bot commented Sep 14, 2020

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

@nlisker
Copy link
Collaborator

nlisker commented Sep 16, 2020

The "New features / API additions" repeats some things already stated. Is it to make each section independent?

@kevinrushforth
Copy link
Member Author

I wanted the "New features / API additions" section to stand on its own. Once thing that might be redundant now is the following sentence in the "Contributing code and documentation changes" section:

"Feature requests, in particular, must be discussed ahead of time and will require significant effort on your part."

I think that could be removed or incorporated in "New features / API additions".

@nlisker
Copy link
Collaborator

nlisker commented Sep 18, 2020

I think that "Contributing to the OpenJFX codebase" should be renamed to something related to a style guide. Then split the testing part to its own subsection.

Also, I still find it confusing that "New features / API additions" is not under the code contribution section. There seems to be 2 main sections: reporting bugs / requesting features - these don't involve code, just talk; then there is contributing code, which covers the process for setup, submissions of bugs fixes, submission of features/API, style, and testing (in some order). Wouldn't this be a better flow?

@kevinrushforth
Copy link
Member Author

Yes, I do think the flow could be better. I'll need to put this on hold for a while, but when I get back to it, I'll look at your suggestions and see if I can come up with something that will improve the flow.

Btw, the thinking behind putting the "New features / API additions" sections at the end (sort of like an appendix) is that I didn't want it to get in the way of the "here's how you sumbit and review a PR" for bug fixes, which is the more common case. I don't think it achieves that in its current form.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@kleopatra
Copy link
Collaborator

kleopatra commented Oct 5, 2020

not sure whether it belongs here, or whether or not it's obviously implied but: I would like to see a bit of clarification on testing of contributions. Right now the sentence might be interpreted to be about running available tests:

Test your changes
Run the test suite to make sure that nothing is broken.

add something like:

  • For most code changes, new tests covering those changes are mandatory.

@Maran23
Copy link
Member

Maran23 commented Apr 29, 2021

Is there any plan to integrate this? I quite like the change and it makes the process much more clear. :)

@kevinrushforth
Copy link
Member Author

Yes, I'll pick this back up soon.

@kevinrushforth
Copy link
Member Author

This has been dormant for a long time, so I just merged the latest master. I also discovered a couple additional changes I had in a "working" branch that I will push to address a couple minor comments.

I still need to get back to this to address the comments from @nlisker about the flow, so that might take a bit more time, but I would like to get this moving to at least fix the fact that several things are wrong or misleading in the current doc.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
1. Renamed "Contributing to...codebase" section to "Coding style and testing guidelines", and switched that section with the "New features section"
2. Updated recommend JDK
3. Added forward reference for Draft and WIP PRs
Copy link
Collaborator

@johanvos johanvos left a comment

Choose a reason for hiding this comment

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

This PR is a major improvement over the existing status.
I therefore recommend to integrate this, and have other issues being discussed in follow-up issues. A contributing guideline document is a moving target.

@kevinrushforth
Copy link
Member Author

I therefore recommend to integrate this, and have other issues being discussed in follow-up issues

Agreed. I think I've addressed all comments made in this PR.

@aghaisas @arapte @nlisker can one of you be the second reviewer?

@nlisker
Copy link
Collaborator

nlisker commented Dec 17, 2021

I will review this shortly.

CONTRIBUTING.md Show resolved Hide resolved
Copy link
Collaborator

@nlisker nlisker left a comment

Choose a reason for hiding this comment

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

I added 1 suggestion.

CONTRIBUTING.md Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Dec 17, 2021

@kevinrushforth This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8231601: Update CONTRIBUTING.md to clarify process for contributing features plus Skara changes

Reviewed-by: nlisker, jvos, mstrauss

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

  • 5422a5a: 8278860: Streamline properties for Monocle

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Ready to be integrated label Dec 17, 2021
@openjdk openjdk bot removed the ready Ready to be integrated label Dec 17, 2021
@openjdk openjdk bot added the ready Ready to be integrated label Dec 17, 2021
@kevinrushforth
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Dec 18, 2021

Going to push as commit 18063ad.
Since your change was applied there has been 1 commit pushed to the master branch:

  • 5422a5a: 8278860: Streamline properties for Monocle

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Dec 18, 2021
@openjdk openjdk bot closed this Dec 18, 2021
@openjdk openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 18, 2021
@openjdk
Copy link

openjdk bot commented Dec 18, 2021

@kevinrushforth Pushed as commit 18063ad.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@kevinrushforth kevinrushforth deleted the 8231601-contributing branch December 18, 2021 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated
7 participants