Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Create a CONTRIBUTING doc #307

Closed
johanvos opened this issue Dec 19, 2020 · 11 comments · Fixed by #433
Closed

Create a CONTRIBUTING doc #307

johanvos opened this issue Dec 19, 2020 · 11 comments · Fixed by #433
Assignees

Comments

@johanvos
Copy link
Contributor

We need to be more clear about how people can contribute. We should be very open to issues, PR's and reviews, but the process needs to be clear.
In general, a PR should refer to an issue, and it should contain a test that fails before the patch, and succeeds after the patch. Also, we should provide guidelines for PRs, so that the review process becomes slightly simpler.

@johanvos johanvos self-assigned this Dec 19, 2020
@abhinayagarwal abhinayagarwal added this to the Scene Builder 2021 milestone Dec 24, 2020
@abhinayagarwal
Copy link
Collaborator

#313 Adds templates to the repository for filing issues and creating PRs

@ghost
Copy link

ghost commented Mar 7, 2021

@johanvos
Further idea regarding contributions: #326

@Oliver-Loeffler
Copy link
Collaborator

Question: How to handle commits? Especially when creating PRs, what kind of commit style do you prefer? Many small commits or one big squashed commit? Also, during code reviews, is it better to make multiple commits until resolution - squash then or don't squash at all?

@abhinayagarwal
Copy link
Collaborator

We have no issues with contributors making multiple commits in a PR. When the PR is finally merged it will be squashed into a single commit.

@Oliver-Loeffler
Copy link
Collaborator

Thanks! Unfortunately more questions come to my mind.

Whats the current process dealing with 'old' issues which are still open but where a PR was merged which closes the issue?
Example: Issue #34 and PR #228 . In general, how is decided, when an issue can be closed?

Also, how are new feature requests/questions handled? Some, smaller things, are kind of handled straight forward - what I observe. What about more structural things? Is there a vision, some kind of overall goal where SceneBuilder shall develop to?

@abhinayagarwal
Copy link
Collaborator

Hi @Oliver-Loeffler ,

Great questions. A clean up of issues and PR is due since we moved the repository to Github. #34 is a great example of why its high time :)

Scene Builder is in great shape and it does what it is meant to do. Gluon is trying to maintain it, quash bugs and accept any community contribution. Similar to OpenJFX.

As you have already noticed, smaller PRs are easy to review and therefore gets in quickly. Larger PR or any new features are difficult to review and therefore takes time.

If you have any suggestions to the overall process, please let us know.

@abhinayagarwal abhinayagarwal removed this from the Scene Builder 2021 milestone Sep 1, 2021
@AlmasB
Copy link
Collaborator

AlmasB commented Jan 8, 2022

@abhinayagarwal

How about something like this, I just repurposed one for FXGL

How to contribute to scenebuilder

Contribution of any form is welcome! Please see the list below on how you can contribute to the project. Once you've decided what you would like to do, let us know about it first in the Discussions section (this needs creating...). This is just to make sure that the issue you want hasn't already been implemented, fixed or being worked on in newer versions. Any new API or changes to existing API should be discussed to avoid inconsistencies.

  • Provide or suggest an implementation of an issue from GitHub Issues.
  • Proof read the public documentation for errors, ambiguities and typos.
  • Crash test features for bugs.
  • Create an issue or suggest a feature backed up by a use case.
  • Add missing tests.

Any Pull Request should follow the provided template.

All Pull Requests will be reviewed in accordance with the Code of Conduct (this needs creating...)

@johanvos
Copy link
Contributor Author

johanvos commented Jan 9, 2022

Good idea. A few remarks:

  • code conventions: maybe we can use the same as OpenJFX: https://github.com/openjdk/jfx/blob/master/CONTRIBUTING.md#coding-style-and-testing-guidelines
  • Discussions section: would GitHub discussions be ok for this, or do we prefer a mailinglist?
  • PR's: I don't think we need long guides, but an important point to stress upfront is that there is more to a PR than just the initial PR. Contributors are highly encouraged to contribute, but we have to take the effort for reviewers and maintainers into account as well.

@AlmasB
Copy link
Collaborator

AlmasB commented Jan 9, 2022

  • code conventions: OpenJFX should do
  • My preference GitHub discussions, as they are much more accessible
  • PRs: the existing template is fine, perhaps a few extra points if needed. With Code of Conduct I didn't mean a guide for PR, but more of a guidance on general development, example

@Oliver-Loeffler
Copy link
Collaborator

Currently we have 2 issue templates, bug and feature request. How about refactorings? If we would like to discuss changes before a PR is started and if we'd like to increase number of tests and test coverage, a refactoring issue template could help.
Or would you go directly ahead with a refactoring PR?

@AlmasB
Copy link
Collaborator

AlmasB commented Jan 11, 2022

Public one-off refactorings tend to lead to long review times, so here a template eventually could be useful.

As for long-term contributors, to expedite the process, at least in the short term, we could agree certain rules, such as refactorings should not affect multiple files or be complex (whatever the definition of this is). As I learn the codebase, most of my PRs in the short term will be small tweaks in the app module. I would also like to begin reviewing simple PRs, please can you point me to some of yours that you think are straightforward to review and merge.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants