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

Add a document with contribution guidelines #275

Merged
merged 1 commit into from
Feb 27, 2016

Conversation

kamalmarhubi
Copy link
Member

This gives us a place to point new contributors, as well as to document
our project practices.

@kamalmarhubi
Copy link
Member Author

@kamalmarhubi
Copy link
Member Author

The part of this I'm really excited about is the issue labels. There are probably more to add, but here's a start!

@kamalmarhubi
Copy link
Member Author

@abbradar as a new contributor, would a document like this be helpful?

@fiveop
Copy link
Contributor

fiveop commented Feb 24, 2016

The [homu] link seems wrong.

@abbradar
Copy link
Contributor

Several more things which might be useful:

  1. Code style (basic things, like "4 spaces").
  2. Policies w.r.t. new APIs that are more or less agreed upon (use libc, provide wrappers to libc structures where it sounds a good idea).
  3. Testing policy (i.e. don't forget to run cargo test at least for your compiler and platform).

(BTW I'll continue working on my patches later, as I'm a bit short of time now)

@kamalmarhubi
Copy link
Member Author

The [homu] link seems wrong.

Fixed, and added a link to the queue as well.

@kamalmarhubi
Copy link
Member Author

@abbradar thanks for the comments!

  1. Code style (basic things, like "4 spaces").

I'm going to leave this out for now. I'm planning to make real progress on https://github.com/nrc/rustfmt/issues/434 and hopefully soon we'll be able to just say "run cargo fmt-diff" or whatever the command looks like.

  1. Policies w.r.t. new APIs that are more or less agreed upon (use libc, provide wrappers to libc structures where it sounds a good idea).

I think these belong in the conventions doc. There's definitely more that needs to go in there though. Thanks for the suggestions!

  1. Testing policy (i.e. don't forget to run cargo test at least for your compiler and platform).

Hm, I thought this would be implied by us having travis-ci set up. How do you think I could make it clearer?

@kamalmarhubi
Copy link
Member Author

Also cc: @posborne :-)

@abbradar
Copy link
Contributor

Travis forces the "tests should pass" policy but from my experience reminding that contributors should run final tests on their own machines somewhere can nevertheless reduce the volume of "Travis fails on this patch; can you please look?" messages from maintainers. An example would be yours truly -- I didn't have much Rust experience so I actually found out about cargo test because Travis has failed on one of my patches. That said, there's good chance it's just me having poor contributing discipline -- I imagine one would look around and find out if there are tests and how to run them for any project they want to contribute to.

@arcnmx
Copy link
Contributor

arcnmx commented Feb 24, 2016

It's probably a decent idea to include a small section on testing. Something that describes how to run the tests and instructs contributors to include tests with their PR when appropriate, etc.

@posborne
Copy link
Member

Agreed. I am working on some additional test infrastructure for several of the more exotic targets (ARM, bsd, ...) and will do my best to come up with a solution that would allow for tests to be run locally (using docker, qemu, etc.) in a way that matches what Travis will be running. This should make it easier to both contribute to those platforms and avoid regressions.

@kamalmarhubi
Copy link
Member Author

@abbradar @arcnmx can you take another look?

@kamalmarhubi
Copy link
Member Author

That said, there's good chance it's just me having poor contributing discipline -- I imagine one would look around and find out if there are tests and how to run them for any project they want to contribute to.

No, your changes were great! We just need better documentation around contributing. :-)

@kamalmarhubi
Copy link
Member Author

@posborne

come up with a solution that would allow for tests to be run locally (using docker, qemu, etc.)

That would be so great.

@abbradar
Copy link
Contributor

Looks good to me!

@arcnmx
Copy link
Contributor

arcnmx commented Feb 24, 2016

Looks good here too!

@fiveop
Copy link
Contributor

fiveop commented Feb 27, 2016

The moment the merge conflict and the build failure (how can either happen, with a completely new file that is not code ...) are resolved I propose to merge this.

This gives us a place to point new contributors, as well as to document
our project practices.
@kamalmarhubi
Copy link
Member Author

I just squashed and rebased. Let's see how the on-behalf-of thing works.

@homu r=@fiveop

@homu
Copy link
Contributor

homu commented Feb 27, 2016

📌 Commit 901f3e5 has been approved by @fiveop

@homu
Copy link
Contributor

homu commented Feb 27, 2016

⌛ Testing commit 901f3e5 with merge 6e99ad6...

homu added a commit that referenced this pull request Feb 27, 2016
Add a document with contribution guidelines

This gives us a place to point new contributors, as well as to document
our project practices.
@homu
Copy link
Contributor

homu commented Feb 27, 2016

☀️ Test successful - status

@homu homu merged commit 901f3e5 into nix-rust:master Feb 27, 2016
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

Successfully merging this pull request may close these issues.

6 participants