Updated contribution guide #753

nephix · 2019-12-17T11:01:58Z

Addresses #186 and notes in iteration 22

codecov · 2019-12-17T11:10:09Z

Codecov Report

Merging #753 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #753      +/-   ##
==========================================
- Coverage   96.44%   96.44%   -0.01%     
==========================================
  Files          90       90              
  Lines        3459     3456       -3     
  Branches      788      792       +4     
==========================================
- Hits         3336     3333       -3     
  Misses         70       70              
  Partials       53       53

Flag	Coverage Δ
#dapp	`92.5% <ø> (-0.03%)`	⬇️
#sdk	`98.13% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
raiden-dapp/src/components/AddressInput.vue	`98.83% <0%> (-0.04%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 721632b...ab30f3a. Read the comment docs.

nephix · 2019-12-17T11:23:01Z

CONTRIBUTING.md


-For CSS naming we try to follow the [BEM](http://getbem.com/) methodology. Along with the naming,
-you should try to scope the styles in their respective components.
+When a reviewer starts a review, he should write a comment in the pull request stating he is doing so. For example:


We can probably get rid of this, since we never do this anyway?

Maybe we should start doing this instead of removing the rule :)

nephix · 2019-12-17T11:23:24Z

CONTRIBUTING.md


-#### Contributing to other people's PRs
+#### Contributing to other pull requests


Not sure if we should keep this

I don't see why we should remove it. While we don't often do it, there were at least a few times I had to actually do it, so it is still useful to be there.

nephix · 2019-12-17T11:24:06Z

CONTRIBUTING.md


 - Self-contained.
 - As short as possible and address a single issue or even a part of an issue.
-  Consider breaking long PRs into smaller ones.
+  Consider breaking long pull requests into smaller ones.


Maybe remove, since we rarely do this too

@nephix Thanks for pointing out, that we are not fully sticking to this rule. I think overall we are doing quite good and improving continuously.

We should stick to it and also address it in PRs (example from google)
Makes things much more transparent and faster. I would keep it and stick to the rule :)

See eg. https://medium.com/letgo/the-art-of-the-small-pull-request-303f7ef63901

The Small PR Manifesto

Our goal is to get PRs integrated onto the master branch as fast as possible. Code isn’t useful until it gets into master.

Here’s why:

In order to make PRs easy-to-read and review, we’ll make them small.

In order to avoid conflicts in our PRs, we’ll make them small.

In order to handle a single responsibility in each of our PRs, we’ll make them small.

In order to promote healthy and constructive discussion in our PRs, we’ll make them small.

In order to make deployments small and easy to grasp in our PRs, we’ll make them small.

In order to have fast PRs, we’ll make them small.

There are also tons of articles, which show the benefit of having small pull requests.

CC @andrevmatos @kelsos

The fact that we don't do it is wrong. Smaller PRs are always easier to comprehend and review. The bigger a PR is the easier it becomes to miss details.

Also, this guide is not only for internal use but also for possible external contributors.
This clearly states intent and requirement and removing it might create wrong assumptions about what is acceptable.

Lets say we do it very rarely.

And I asked whether we should remove it because it doesn't make sense to write down rules and expect others to follow them while we do what suites us

nephix · 2019-12-17T11:25:41Z

CONTRIBUTING.md

+- **Ready for review**: Means that you consider your work done and it is currently waiting for a review
+- **Draft**: Is considered work in progress and it is not yet ready for a review


Maybe remove, since we never adhere this

We don't even have those labels do we? 🤓

@taleldayekh we removed the labels and instead rely on the Draft and Ready for review feature of the Github Pull Requests

Ah, I guess you set them when opening the PR.

@nephix Can we clarify here that these are not labels, but PR status in the Github PR interface. I actually adhere to this and @andrevmatos is also doing the same from what I remember.

The regular flow should be, I open a draft PR and when I am done I mark it ready for review.

From my experience we don't do this at all, or it's at least inconsistent.
I mostly do draft/ready for review, but I often see pull requests in ready for review with [wip] or things like these.
I also very often receive feedback on pull requests that are in draft state.
Because of that, I asked if we should remove it or if we should talk about and align on this.

Before removing maybe we should definitely align and discuss them. However, since discussions take time maybe it is better to clarify the original intent and keep it for now, but also add a point for discussion after everyone will be back.

CONTRIBUTING.md

kelsos · 2019-12-17T16:19:01Z

CONTRIBUTING.md


-## Development environment setup
+```html


@nephix don't we also enforce this with eslint? If this is part of the eslint configuration maybe we should remove it.

I don't know, could be. I just left it inside because I wasn't sure

@nephix #599 :P

Cool, time to get rid of it :)

taleldayekh

Sehr gut!

kelsos

Some clarification is needed but in general, it looks good. Thank you very much.

kelsos

@nephix I am approving and you can merge. We can look into the points after the holidays.

nephix force-pushed the chore/updated-contrib-guide branch 2 times, most recently from cc90874 to 208fca7 Compare December 17, 2019 11:21

nephix commented Dec 17, 2019

View reviewed changes

nephix requested a review from kelsos December 17, 2019 11:24

nephix marked this pull request as ready for review December 17, 2019 11:24

nephix commented Dec 17, 2019

View reviewed changes

nephix requested review from andrevmatos and taleldayekh December 17, 2019 11:25

nephix force-pushed the chore/updated-contrib-guide branch from 208fca7 to 35ff00c Compare December 17, 2019 12:16

Updated contribution guide

ab30f3a

nephix force-pushed the chore/updated-contrib-guide branch from 35ff00c to ab30f3a Compare December 17, 2019 12:19

taleldayekh reviewed Dec 17, 2019

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

taleldayekh reviewed Dec 17, 2019

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

taleldayekh reviewed Dec 17, 2019

View reviewed changes

CONTRIBUTING.md Show resolved Hide resolved

kelsos reviewed Dec 17, 2019

View reviewed changes

taleldayekh approved these changes Dec 17, 2019

View reviewed changes

kelsos suggested changes Dec 17, 2019

View reviewed changes

kelsos approved these changes Dec 17, 2019

View reviewed changes

nephix merged commit a845236 into raiden-network:master Dec 17, 2019

nephix deleted the chore/updated-contrib-guide branch December 17, 2019 16:51

christianbrb mentioned this pull request Dec 17, 2019

Reviewing and deciding on PR guidelines #758

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated contribution guide #753

Updated contribution guide #753

nephix commented Dec 17, 2019

codecov bot commented Dec 17, 2019 •

edited

Loading

nephix Dec 17, 2019

kelsos Dec 17, 2019

nephix Dec 17, 2019

kelsos Dec 17, 2019

nephix Dec 17, 2019

nephix Dec 17, 2019

christianbrb Dec 17, 2019 •

edited

Loading

kelsos Dec 17, 2019

nephix Dec 17, 2019 •

edited

Loading

nephix Dec 17, 2019

taleldayekh Dec 17, 2019

kelsos Dec 17, 2019

taleldayekh Dec 17, 2019

kelsos Dec 17, 2019

nephix Dec 17, 2019

kelsos Dec 17, 2019

kelsos Dec 17, 2019

nephix Dec 17, 2019

kelsos Dec 17, 2019

nephix Dec 17, 2019

taleldayekh left a comment

kelsos left a comment

kelsos left a comment


		#### Contributing to other people's PRs
		#### Contributing to other pull requests

		- Ready for review: Means that you consider your work done and it is currently waiting for a review
		- Draft: Is considered work in progress and it is not yet ready for a review

Updated contribution guide #753

Updated contribution guide #753

Conversation

nephix commented Dec 17, 2019

codecov bot commented Dec 17, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianbrb Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nephix Dec 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taleldayekh left a comment

Choose a reason for hiding this comment

kelsos left a comment

Choose a reason for hiding this comment

kelsos left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2019 •

edited

Loading

christianbrb Dec 17, 2019 •

edited

Loading

nephix Dec 17, 2019 •

edited

Loading