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

Better Guidelines for Contributing #156

Closed
bjubes opened this issue Aug 18, 2016 · 16 comments
Closed

Better Guidelines for Contributing #156

bjubes opened this issue Aug 18, 2016 · 16 comments

Comments

@bjubes
Copy link
Collaborator

bjubes commented Aug 18, 2016

I am suggesting that a section be added to the contribution guidelines that outlines more specifically the process of PR, code expectiations, and best practices. So far my list of additions is:

  • Before you start coding, open an issue so that the community can discuss whether your change is needed or in line with the goals of the project. This allows discussion and results in a more succent and focused additions
  • Pull Requests represent final code. That means they must be:
    • well tested by the author. We curently have only quill testing PR, so it is the authors job to ensure their code works as expected.
    • Be free of unneesicary log calls (Log Spam and Practices #94). Logging is great for debugging but when a PR is made log calls should only be called when there is an actual error or to warn of an unimplemented feature
  • Small changes are preferable over large ones. The larger a change is the more likely it is to conflict with the project and thus be denied. Be sure to extensively discuss large additions, as no one wants to spend hours coding just to see their PR denied.
  • document your change in your PR. If you add a feature that you expect others to use, explain exactly how future code should interact with your code.
  • don't change the scene unless nessicary. Scene changes easily conflict and are difficult to resolve. Do not delete or rename anything in the scene.
  • include pictures in PR for visual changes. (Already in contributing file but prioably move it to this new section)
@bjubes
Copy link
Collaborator Author

bjubes commented Aug 18, 2016

please tell me which ones your think are good, what should be changed, and any rules I missed

@emilkris33
Copy link
Contributor

All are very good.

@Tranberry
Copy link
Contributor

As long as the guidelines isn't a huge obstacle for new coders and contributors this is all good. I know of project with guidelines so menacing, that people won't share their work just due to the backlash from people complaining about minor errors.

All these ideas seems good to me.

bjubes referenced this issue Aug 18, 2016
Added some best-practice notes.
@alexanderfast
Copy link
Collaborator

@Tranberry If someone sits down and spends time giving feedback on your work you shouldnt see it as backlash. And its the minor errors that are easy to fix. Having "menacing" guidelines saves everyone time in the end, everyone should be clear on the expectations to cut down on the back and forth.

@bjubes
Copy link
Collaborator Author

bjubes commented Aug 18, 2016

Yeah I don't want to constrict people. I just want to set a few ground rules so that people understand why their PR don't get accepted. I think putting this under a header called "Guidelines" and then add a link that says "before you start, read the guidelines"

@Tranberry
Copy link
Contributor

Tranberry commented Aug 18, 2016

I have no problem with that @Mizipzor I value people (like you) taking their time to explain things to us who don't know. I do have a problem with guidelines using obscure words and being incredibly nitpicky. Here there are no such problems.

I believe these points are all good.


I'm wondering how I could have said that in my original comment, I don't like to sound like the complaining idiot when I try to raise awareness of things.

@alexanderfast
Copy link
Collaborator

@quill18 You have been closing a few pull requests today that had conflicts. But those conflicts can be fixed and the pull request can be updated merely by pushing more commits.

If the policy is to either merge or close as to not leave anything lingering I feel this should be expressed in the contribution guidelines. The concerns in this thread regarding the feelings of rejection are very valid. Especially as this project kinda targets newcomers.

@SanteriHetekivi
Copy link
Contributor

For me the more guidelines and rules the better, but I can see that for others and for new contributors they may be seen as limiting.

In my mind @bjubes 's list is great and pretty relaxed.

@vogonistic
Copy link
Collaborator

Are we sure the first point is needed? If you want to change a subsystem or make something big, I think it makes a lot of sense, but if you just found a bug or want to add something small, I'd probably just go for it and submit the PR.

Maybe when we have a chat channel we can ask people to check there if someone else is already working on something similar.

@svmnotn
Copy link
Contributor

svmnotn commented Aug 18, 2016

I think for larger changes (like the localization, a power system, or a whole new UI implementation) it would be nice to do something like an RFC (Request for comment) so that the whole idea can be discussed in detail before any work is done so as to make sure there isn't a need for 10 other PRs afterwards just fixing minutia. For example, the Rust Programming Language has a rather nice RFC procees.

@vogonistic
Copy link
Collaborator

@svmnotn That's a pretty nice and straightforward RFC format. It'd be cool to have something similar, but I don't know if I see the need for a separate repo for it.

@svmnotn
Copy link
Contributor

svmnotn commented Aug 18, 2016

I was just trying to point out the format, since a game should not have as many Major changes that would require an RFC, I also think there really isn't a point to have a separate repo, maybe just have an RFC tag for issues?

@vogonistic
Copy link
Collaborator

I like it. A label or just putting RFC in the title could be a good start to see how it works. Some of the things that could have been informal RFCs are #147, #86 and #153 off the top of my head.

@bjubes
Copy link
Collaborator Author

bjubes commented Aug 18, 2016

Yes. I think a label would be the easiest solution for now, just to make sure that huge project changes are discussed and seen by all so no one is surprised when a huge change is merged in and the code now interacts very differently.

@bjubes
Copy link
Collaborator Author

bjubes commented Aug 19, 2016

Made a pull request with actual changes in the contributing file #220
try to discuss this further on that PR so i can make changes as they are suggested

@vogonistic
Copy link
Collaborator

I'm closing this issue since we merged the PR. Open a new issue if there are more suggestions.

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

No branches or pull requests

7 participants