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 CONTRIBUTING guide #2087

Merged
merged 3 commits into from Oct 31, 2017
Merged

Adding CONTRIBUTING guide #2087

merged 3 commits into from Oct 31, 2017

Conversation

andreslucena
Copy link
Member

Is the last thing we need for having completed all the items on the Github community checklist:

https://github.com/decidim/decidim/community

@codecov
Copy link

codecov bot commented Oct 21, 2017 •
edited

Codecov Report

Merging #2087 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2087   +/-   ##
=======================================
  Coverage   98.55%   98.55%           
=======================================
  Files        1180     1180           
  Lines       27009    27009           
=======================================
  Hits        26620    26620           
  Misses        389      389

@mrcasals
Copy link
Contributor

mrcasals commented Oct 23, 2017 •
edited

@andreslucena codeclimate is complaining about trailing whitespaces in lines 37, 41 and 45:

https://codeclimate.com/github/decidim/decidim/pull/2087

Can you fix this? 😄

@ghost ghost assigned andreslucena Oct 23, 2017
@ghost ghost added the in-progress label Oct 23, 2017
@andreslucena
Copy link
Member Author

@mrcasals fixing it, I'm using a new editor that I need to fix. Thanks!

mrcasals
mrcasals previously approved these changes Oct 23, 2017
View reviewed changes
deivid-rodriguez
deivid-rodriguez previously approved these changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez 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 a couple of minor comments, but looks great!

CONTRIBUTING.md Outdated
#### **Did you fix whitespace, format code, or make a purely cosmetic patch?**

Changes that are cosmetic in nature and do not add anything substantial to the stability, functionality, or testability of Decidim will generally not be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want this whole point. Our code is 100% properly formatted, and follows strict style rules, so this is a non-issue in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

it's from Rails, they also have an issue with the rationale behind this:
rails/rails#13771 (comment)
https://github.com/rails/rails/blob/master/CONTRIBUTING.md

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm aware, I just don't think it applies to us.

Rails has almost 5K all time contributors and 750 open PR's, it's an unusually highly active project for external contributions. I'm sure part of why they introduced that rule is because they were struggling to handle that.

In Decidim we don't have that problem and, at least since I'm around, we've been following something closer to the "no patch is too small" philosophy :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like some more feedback regarding the cosmetic changes. @decidim/developers what do you think about these lines?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm 👍 to what @deivid-rodriguez says here, at least for now. Apart from being properly formatted, our code can have some refactors and this part can seem like we won't accept refactors

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll remove this lines, pefect!

CONTRIBUTING.md
* Suggest your change in [Metadecidim](https://meta.decidim.barcelona/processes/roadmap) and start writing code.

* Do not open an issue on GitHub until you have collected positive feedback about the change. GitHub issues are primarily intended for bug reports, fixes and already discussed features on [Metadecidim](https://meta.decidim.barcelona/processes/roadmap).

Copy link
Contributor

Choose a reason for hiding this comment

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

I love the idea of using decidim itself to discuss features ❤️. As a matter of fact, I just registered at meta.decidim.barcelona. 😄

However, is it appropriate to link to a Catalan/Spanish only website from GitHub?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could use the decidim.org domain and host a general installation there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, but I guess it's easier for now to keep accepting feature requests via GitHub issues as well, and point there here (or maybe point to both places).

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK the plan in the short-midterm is to use english in metadecidim 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

English on Metadecidim will come soon, sure. this issues we are going to discuss at the JAM meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! 👍

mrcasals
mrcasals reviewed Oct 27, 2017
View reviewed changes
CONTRIBUTING.md

#### **Did you find a bug?**

* **Do not open up a GitHub issue if the bug is a security vulnerability in Decidim**, and instead send us an email to security [at] decidim.org.
Copy link
Contributor

Choose a reason for hiding this comment

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

@andreslucena does the email address security [at] decidim.org exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it works

mrcasals
mrcasals previously approved these changes Oct 30, 2017
View reviewed changes
@ghost ghost assigned mrcasals Oct 30, 2017
@mrcasals
Copy link
Contributor

Rebasing against master to fix the builds.

@mrcasals
Copy link
Contributor

Re-rebasing again to fix tests :(

@mrcasals mrcasals merged commit 73065db into master Oct 31, 2017
@mrcasals mrcasals deleted the docs/CONTRIBUTING branch October 31, 2017 15:02
@ghost ghost removed the in-progress label Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josepjaume josepjaume josepjaume left review comments

@xabier xabier xabier left review comments

@deivid-rodriguez deivid-rodriguez deivid-rodriguez left review comments

@mrcasals mrcasals mrcasals approved these changes

Assignees

@mrcasals mrcasals

@andreslucena andreslucena

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@andreslucena @mrcasals @josepjaume @xabier @deivid-rodriguez