Add maintainer guidelines #9315

danpetry · 2018-06-08T14:16:25Z

Contribution description

This PR adds a set of maintainer guidelines.

The aim is to provide a helpful set of steps and guidelines to assist maintainers, both to facilitate their job, and to encourage a high standard of review across the board.

It would be useful to gain input from as many other maintainers as possible as to what the specifics of "a high standard of review" are, so they can be captured here.

It is to be noted that these are not a set of "rules", that must be enforced. They are rather guidelines, and the implementation should end doing the two things in the second paragraph, not end up hindering people.

danpetry · 2018-06-08T14:18:01Z

Previous comments from discussion on splinepad are below:

unnamed: any ideas how to concret "good reasons"?

unnamed: do 1.3 and 1.4 target the same "issue"?

unnamed: 1.5: does this mean transperency on how to reproduce and est this one PR? e.g., if a special setup is needed, how to set this up? maybe we can concretise this point

unnamed: 2.3: guess we need to practice better here and might introduce sme toolse in future?

unnamed: maybe add some cross-references/links to the wiki, further explaining doc conventions, for example ...

Thomas: good reasons -> present striking/justifying arguments

miri64

unnamed: maybe add some cross-references/links to the wiki, further explaining doc conventions, for example ...

+1

miri64 · 2018-06-08T16:00:59Z

MAINTAINING.md

+1.2 Do the concepts used by the PR make sense? 
+1.3 Does the PR break with existing concepts?
+1.4 Is the structure of the PR itself valid?
+1.5 Are the testing criteria clear?


unnamed: 1.5: does this mean transperency on how to reproduce and est this one PR? e.g., if a special setup is needed, how to set this up? maybe we can concretise this point

I agree with this comment (it might be my own, but I'm not sure). IMHO this point should not mean "Contributor, please provide a test applications or scripted test case" (unless they are really straight forward or prove that the PR actually improves upon the current code base or because the contributor wants to). This could (and should) e.g. interpreteted as that the API (inkl. documentation) is clear enough, that a third party can implement comprehensive tests (ideally black box) on their own. Or in case of a bug fix of course: A description how to reproduce the bug and how the PR fixes that.

And yes, this is a diversion from our current handling of tests.

Perhaps it could read, "Are there clear and adequate instructions on how to test the PR or reproduce the bug" and perhaps 1.6, "Are any design and/or API changes clearly documented"?
It seems that these points are phrased such that they are open to interpretation - in other words, there's a certain degree of trust placed in the Maintainer. Which I think is appropriate.

ZetaR60 · 2018-06-09T02:11:21Z

Regarding "good" reasons: in law you might define this as something like "a reasonable person, skilled in the art, would believe that the reason gives sufficient justification". A "reasonable person" is a legal fiction for how the community believes a typical personal in that position ought to behave. A person skilled in the art (which is programming in this case) is a similar legal fiction of a typical person with normal skills of a professional in that field (is not incompetent and is not a genius). Concepts like "reasonable person" are a convenient way of dealing with hard to define problems in imperative language by letting the community deal with instead. This is a bit verbose for the document, but it might be a conceptual starting point for deciding how to word it.

danpetry

I've added some comments of my own both as placeholders for myself and in response to/to stimulate discussion. My plan is to wait until the end of the week and then make changes after discussion

danpetry · 2018-06-11T08:03:49Z

MAINTAINING.md

+1.2 Do the concepts used by the PR make sense? 
+1.3 Does the PR break with existing concepts?
+1.4 Is the structure of the PR itself valid?
+1.5 Are the testing criteria clear?


Perhaps it could read, "Are there clear and adequate instructions on how to test the PR or reproduce the bug" and perhaps 1.6, "Are any design and/or API changes clearly documented"?
It seems that these points are phrased such that they are open to interpretation - in other words, there's a certain degree of trust placed in the Maintainer. Which I think is appropriate.

danpetry · 2018-06-11T08:16:46Z

MAINTAINING.md

+
+## 1. - Review the fundaments
+
+1.1 Does the reasoning for this PR make sense?


1.0 Does it compile and run as expected in a basic fashion?

IMO the 1.0 aspect should be closer to 1.5 and not next to the conceptual question

ok cool. So 1.6 could be "Do the tests run and pass?"

danpetry · 2018-06-11T08:17:29Z

MAINTAINING.md

+2.4 Check for API compliance
+2.5 Check for consistent error handling
+2.6 Check scope of variables and functions
+2.7 Check for programming errors


Perhaps this could be phrased as "syntactical, semantical or logical errors, particularly ones that may lead to runtime errors"

+1 for "Check for syntactical, semantical or logical errors"

danpetry · 2018-06-11T08:28:25Z

MAINTAINING.md

+## Detailed Rationales, Explanations, and Examples
+
+WIP: extend this!
+


Note to self: language needs to be made more formal in places

I have the impression that following sections should be somehow polished (e.g., "cppcheck should find this, right?)")

danpetry · 2018-06-11T08:30:39Z

MAINTAINING.md

+### On 1.
+
+#### On 1.3
+- does this code properly maintain the Git history of previous contributions? (note by Martine: a PR can't change the Git history of the mainline repo => no force push possible, but maybe something else is meant here, if so, please specify)


Change to 1.6 and clarify that this means to keep the Git history of multiple authors if multiple authors are involved

danpetry · 2018-06-11T08:36:32Z

MAINTAINING.md

+- can all code paths be actually reached?
+    - e.g. always `true/false` conditions
+- are all critical sections (`lock/unlock`, `acquire/release`, ...) always
+  closed on every code path?


Do we use any code coverage tools at the moment?

danpetry · 2018-06-11T08:42:47Z

MAINTAINING.md

+baseline on things that should be checked.
+
+Note: Any of the items in this list might be skipped, if **good** reasons are presented.
+


"Good reasons" could be replaced by "reasons that are clearly and logically articulated"

danpetry · 2018-06-11T08:44:14Z

MAINTAINING.md

+
+
+## 3. - Test the PR
+Run tests to verify the correct behavior (see 1.5), or present **good** reasons


As above wrt use of the word "good"

Also, would it be worth mentioning that testing should be done not just on native, but also a couple of different boards incl. our reference board?

danpetry · 2018-06-11T08:44:49Z

MAINTAINING.md

+4.1 Check for sufficient high-level (module-level) documentation
+4.2 Verify function level documentation
+4.3 Are critical/hard to understand parts in the code documented?
+4.4 Check coding style (w.r.t. the coding conventions)


Hyperlink to coding conventions

Maybe also link best practices?

I think this should be moved to part 2. "Review the code line-by-line". I don't think reviewing code that doesn't follow coding conventions is possible. And it's easy to say: your contribution is not compliant with RIOT coding conventions or please run uncrustify.
I would also rename this part "Review documentation" since it's mostly related to this.

I disagree with that. Yes, coding style is important and that's why it appears in this list in the first place and yes, checking coding style is easy. But it doesn't make sense to comment on coding style if the overall concept of the PR doesn't make any sense. Imagine there is a new PR by a new contributor and you go at it comment on all the coding style issues. Then you (or someone else) is looking what the now most beautiful code is actually doing, and realize it's actually total bullshit. Either, the contributor was already annoyed by your nitpickyness and now is even more annoyed that close the PR for valid reasons or they diligently followed your instructions and you wasted the contributor's and your time, because the PR isn't any good. The PR should be well understood (2 and 4.1-3) and tested (3) before any comments on coding style are made.

(+ @jia200x work on automating uncrustify should reduce the need to comment on any glaring coding style issues ;-))

But it doesn't make sense to comment on coding style if the overall concept of the PR doesn't make any sense

Part 2 is not on overall concepts but rather on code, that's why coding style should go there.

Okay, yes that was my mistake. 2 is about code correctness, which groups it rather with (3) than with 4.1-3. But my point stands, that the subjective* code style should take no precedence over the objective code correctness and concepts.

Yes there is a document that describes the rules we agreed upon for style. Nevertheless, these rules are still based on subjective opinion which a contributor might not share (at least for their day to day work) but needs to adapt to.

@aabadie basically I understand the reasoning re efficiency of reviewing. Uncrustifying code as a "first check" is low on maintaining time and people should have done this anyway. Also, sometimes people submit something where the code guidelines are simply not followed which makes it really difficult/feel rather futile to review. It sounds like this is where you're coming from here? I would say, that in a particular case, it might be judicious to deviate from these guidelines. If someone submits something which is just difficult to review because it's really poor, then of course it would be the maintainer's freedom to exercise good judgement and say, "please clean this up before reviewing". Maybe giving some general guidance as to how, for example run uncrustify or whatever else.
IMO, as a broad generalization (i.e. not taking into account these individual cases), the general philosophy/flow of reviews should be high level -> details. Reason being, that if we do code review, and then the higher level structure of the code or some other design aspects need to be changed, we'll have to do code review again anyway. So, we'll always have to do that code review once (whether it comes first or last), but we'll have to do it twice if design aspects need changing.
I would say that section 2. might be renamed "review design of code" or something like that.

Also, sometimes people submit something where the code guidelines are simply not followed which makes it really difficult/feel rather futile to review. It sounds like this is where you're coming from here?

This is exactly what I have in mind. Why starting to review a code (line-by-line) if it doesn't follow the basic coding conventions of RIOT ? In any case, the contributor will have to, at least, uncrustify it, which might totally change the diff. And previous comments will becomes harder to track via github. Here I make the assumption that several people can review at different moment and also that another pass of review will raise other comments which is generally the case).

So I won't talk of code style in the document, but rather coding conventions and again move it to "review code line-by-line". Maybe this should be done even before looking at the code design. Why ? because this ensures that the contributions "looks like" the existing RIOT's code. And maintainers are used to look at this code, you can better concentrate on the real code design/efficiency problems.

Maybe giving some general guidance as to how, for example run uncrustify or whatever else.

This would be very useful of course. And should be added to the contributing guidelines.

Would be good to differentiate between "coding style" (subjective) and "coding conventions" (absolute). Also to suggest that the placement of applying of "coding conventions" in practice depends on whether the PR comes in in a reviewable state with respect to code cleanliness.

danpetry · 2018-06-11T10:31:23Z

@ZetaR60 my feeling is that, as a set of guidelines for people to use individually, some fairly specific guidance would be appropriate in this respect... maybe "a reasonably competent person" and a slant towards community discussion about this point might not be so helpful for each one person. What do you think of the suggestion made in the review comments?

ZetaR60 · 2018-06-11T18:43:39Z

@danpetry I guess my point is that some of these criteria (esp. "good reasons") are sufficiently complex and organic that making them explicit in formal language is infeasible. There may be "reasons that are clearly and logically articulated" that are also unsuitable, because they are not "good reasons" according to unspoken community standards. I think there are two approaches to this: law has recognized this problem and has specific formal tools for handling it ("reasonable person" tests), or you might leave the language loose with the implication of more complex criteria outside of the guidelines document (I think "good reasons" actually does pretty well for this approach). It may be unhelpful of me to bring up this complex issue though, so if you feel you have an adequate solution then feel free to ignore these suggestions. I just have an interest the nature of logic and formal language and I don't get to use it often.

PeterKietzmann · 2018-06-12T11:28:33Z

MAINTAINING.md

+2.5 Check for consistent error handling
+2.6 Check scope of variables and functions
+2.7 Check for programming errors
+2.8 Check for code efficiency


Though I don't know which efficiency is meant here, that point seems a bit generic to me. Above was already mentioned the code duplication or memory efficiency so this refers to run efficient programming or run time efficiency? Do we want to mention that?

Yep that sounds like it, could replace "code efficiency" with "run time efficiency".

I'd be careful not to engage in efficiency discussions too early. If the code runs, efficiency is something that can be improved later.

Hmm yeah. I guess with this point, it's a balance. Could be phrased, "check that code doesn't have any obvious, unnecessary inefficiencies"? I.e. we're checking here for things that are plain wrong, eg copying a whole struct to a function instead of handing a pointer?
The rule of thumb could be, things that could be improved without changing the overall design of the code?

We should com up with and point to a document defining some best-practices.

There are things that we'd do differently than if we'd not be in tight embedded space.
E.g., if docs specify that argument three needs to point to a buffer that can hold N bytes, we only "assert()" whether the pointer is NULL instead of returning an error at runtime for production builds. That goes against safety / reliability best practices for any serious C development on "real" computers, but is necessary for us in order to keep code size down.

Maybe add something like "(See best practices here: TBD)" would make sense?

"(See best practices here: TBD)"

I've just seen the best practices wiki entry that @PeterKietzmann linked. Down the line, that should have an "efficiency" part that we could link to.

+1 for developing the "best practices". This could be a useful focal point to develop our shared understanding of design priorities

PeterKietzmann · 2018-06-12T12:10:18Z

MAINTAINING.md

+    - `int/unsigned` is only 16-bit on msp430 and avr8...
+
+#### On 2.8
+- make sure we are not wasting any memory trough inefficient coding:


PeterKietzmann · 2018-06-12T12:14:42Z

MAINTAINING.md

+int bar(foo_t v) {
+    int abc = v;
+    ...
+}


I'm wondering if such concrete examples should rather go to the coding conventions or best practices

Yeah that sounds appropriate to me. That way, developers can also benefit from them. Perhaps all "detailed rationale" stuff could, rather than being here, be additions to our current coding conventions/best practices, for this reason? @haukepetersen any opinion?

jcarrano · 2018-06-15T08:27:40Z

MAINTAINING.md

+(1.7 Is the PR a duplicate of another PR?)
+
+
+## 2. - Review the code (line-by-line)


How many of these checks could be automated?

Maybe we can put a note saying that the checks are in the process of/should progressively be automated? Using Cochinelle? And a similar note in section 4.4 about uncrustify?

jcarrano · 2018-06-15T08:29:18Z

MAINTAINING.md

+
+## 4. - Review code style and documentation
+
+4.1 Check for sufficient high-level (module-level) documentation


+1 here. No module-level docs means nobody will be able to find it.

aabadie · 2018-06-15T09:38:17Z

I have a general comment regarding the document: it's missing behavioural guidelines to maintainers/reviewers.

It would be nice to add a paragraph at the beginning explaining several things:

Use GitHub review interface efficiently, if one has several comments, avoid the "Add single comment" button => with will minimize the number of email sent to listeners/people involved
If you requested changes, be responsive and ensure that you are listening to notifications on the PR to avoid blocking it forever (I have good examples :) )
Give precise and good advices when possible and if you think it will help the contributor: code snippet, links to code/web/issues/PRs, etc
Be friendly/objective in your review and comments

And there might be other things to add that I miss.

danpetry · 2018-06-15T11:49:24Z

MAINTAINING.md

+1.1 Does the reasoning for this PR make sense?
+1.2 Do the concepts used by the PR make sense? 
+1.3 Does the PR break with existing concepts?
+1.4 Is the structure of the PR itself valid?


Having not reviewed that much yet, I'm not sure what this means. Does this mean e.g. not combining completely unrelated contributions, or half-contributions, and that kind of thing?

danpetry · 2018-06-15T11:52:52Z

MAINTAINING.md

+
+
+## 3. - Test the PR
+Run tests to verify the correct behavior (see 1.5), or present **good** reasons


Also, would it be worth mentioning that testing should be done not just on native, but also a couple of different boards incl. our reference board?

danpetry · 2018-06-18T08:37:11Z

MAINTAINING.md

+1.5 Are the testing criteria clear?
+(1.6 Does this PR respect the rights of previous authors?)
+(1.7 Is the PR a duplicate of another PR?)
+


Is the solution presented in the PR as simple as possible to satisfy the requirements, but no simpler? [link to Einstein quote]

danpetry · 2018-06-18T08:38:51Z

MAINTAINING.md

+1.5 Are the testing criteria clear?
+(1.6 Does this PR respect the rights of previous authors?)
+(1.7 Is the PR a duplicate of another PR?)
+


1.x Are the requirements and design goals for the PR clearly thought out and clearly expressed?

danpetry · 2018-06-18T08:42:31Z

MAINTAINING.md

+2.5 Check for consistent error handling
+2.6 Check scope of variables and functions
+2.7 Check for programming errors
+2.8 Check for code efficiency


+1 for developing the "best practices". This could be a useful focal point to develop our shared understanding of design priorities

danpetry · 2018-06-18T08:46:09Z

MAINTAINING.md

+in this order. This list is however not exhaustive and defines a rather minimal
+baseline on things that should be checked.
+
+Note: Any of the items in this list might be skipped, if **good** reasons are presented.


Futher note: this is a working document and any changes/discussions/etc are encouraged, via PRs raised against the document, where discussions on the point can be undertaken

kaspar030 · 2018-06-19T07:21:19Z

Should we add best practices on how to communicate, and the actual procedures? I'd rather not see "be nice! don't be racist!" (e.g., not duplicate the CoC), but things like "acknowledge replies to concerns (short "ok!" is fine")", "when done reviewing, write ACK, but mention if review was only of a part, e.g., "untested ACK." or "ACK for coding style, did not think about architecture"". Or is that out of scope?

kaspar030 · 2018-06-19T07:21:46Z

Or is that out of scope?

(or out of scope of this initial version).

danpetry · 2018-06-19T12:33:54Z

Agree with "non-technical guidelines". Would add:

If you don't have time to review someone, that's ok, but we should try and reply to them at least, to say thanks for the awesome contribution and we'll get back to them when we can
Also to put a link to the CoC document but then keep CoC specifics out of the document otherwise.
And put a link to the maintainer wiki page somewhere on it.

danpetry · 2018-06-19T15:52:31Z

I have made the changes suggested. Some notes

All of the specific examples have been moved to the "Best Practices", and a note has been put here saying that those best practices should be followed. That means that developers can benefit from this info too
The conversation seemed to have more or less died down, so, it would be great to tweak final details and get it merged rather than opening up whole new threads of conversation at this stage, if there are no major blockers. The idea is that this is a working document anyway, and that we can update it dynamically and have productive discussions about aspects of maintenance via further PRs. I hope we do.

aabadie

@danpetry, the new items content is good. Maybe you could split the lines to make them fit in 80 characters line length ?

danpetry · 2018-06-22T13:56:48Z

MAINTAINING.md

+The following list is not exhaustive and addresses the coding issues we have regularly seen in the past. In particular, check that the [Best Practices for RIOT Programming](https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming) are followed. These checks can be aided (but not replaced) by a tool such as Coccinelle, using the script found in dist/tools/coccinelle.
+
+2.1 Check for code duplication  
+2.2 Check memory usage  


Include https://github.com/RIOT-OS/RIOT/wiki/Comparing-build-sizes once tested if it works

danpetry · 2018-06-22T13:57:29Z

MAINTAINING.md

+2.6 Check scope of variables and functions  
+2.7 Check for syntactical, semantical or logical errors  
+2.8 Check for any runtime efficiency improvements that can be made in specific lines of code - i.e., without changing the overall design of the code  
+


2.9 Check that the code design is as simple as possible, but no simpler

miri64

I agree with @aabadie about the line-length (helps also with the review, as sentences are easier to sub-divide).

Nit-picky git usage style comment: usually an imperative commit summary style should be used, as if giving a command to the codebase, e.g. "doc: add maintainer guidelines"

miri64 · 2018-06-22T19:06:36Z

MAINTAINING.md

+
+### 1. - Review the fundamentals
+
+1.1 Does the reasoning for this PR make sense? Are the requirements and design goals clearly thought out and clearly expressed? Is the problem that the PR intends to solve clearly stated?  


Lot's of trailing whitespaces here and below

There are two spaces at the end of each line of the numbered lists, so that they print as a list rather than just one long line

I have removed these and changed the lists to integer numbering, i.e. 1, 2, 3 etc. This removes the need for trailing whitespace

miri64 · 2018-06-22T19:09:38Z

MAINTAINING.md

+
+- Usage of labels
+- Division of review responsibilities, e.g. with ACKs for different areas of review
+- Usage of Github functionality re "Reviewers" and "Assignees"


Canonical spelling is GitHub.

miri64 · 2018-06-22T19:10:01Z

MAINTAINING.md

+
+- Usage of labels
+- Division of review responsibilities, e.g. with ACKs for different areas of review
+- Usage of Github functionality re "Reviewers" and "Assignees"


re means "regarding/in the matter concerning" - https://en.oxforddictionaries.com/definition/re

Have changed this for clarity

miri64 · 2018-06-22T19:15:38Z

MAINTAINING.md

+
+Notes:
+- Any of the items in this list might be skipped, if clearly and logically articulated reasons are presented.
+- The order of the steps is meant to maximize efficiency and minimize overhead, with the philosophy that failing in step n makes steps n+x obsolete. However, this efficiency can depend on the quality of the submission itself. If the PR is clearly not in a reviewable state, for example due to poor code cleanliness or overall design, it might be more efficient to give the contributor some broader comments for improvement before further review.


Maybe use parenthesis or hyphen for the insertion "for example due to poor code cleanliness or overall design" to make this rather long sentence easier to parse.

miri64 · 2018-06-22T19:17:14Z

MAINTAINING.md

+
+Notes:
+- Any of the items in this list might be skipped, if clearly and logically articulated reasons are presented.
+- The order of the steps is meant to maximize efficiency and minimize overhead, with the philosophy that failing in step n makes steps n+x obsolete. However, this efficiency can depend on the quality of the submission itself. If the PR is clearly not in a reviewable state, for example due to poor code cleanliness or overall design, it might be more efficient to give the contributor some broader comments for improvement before further review.


This is the first occurrence of the abbreviation "PR". Though maintainers most likely should know what a PR is, I think it's more easier to follow, if PR is resolved here at least: "PR (Pull Request)".

miri64 · 2018-06-22T19:20:19Z

MAINTAINING.md

+
+### 1. - Review the fundamentals
+
+1.1 Does the reasoning for this PR make sense? Are the requirements and design goals clearly thought out and clearly expressed? Is the problem that the PR intends to solve clearly stated?  


These three questions look a little bit busy in the rendered form. Maybe split them up linewise?

1.1 Does the reasoning for this PR make sense? \ Are the requirements and design goals clearly thought out and clearly expressed? \ Is the problem that the PR intends to solve clearly stated?

(apply this for other multi-question points also if you agree)

miri64 · 2018-06-22T19:21:59Z

MAINTAINING.md

+
+The following list is not exhaustive and addresses the coding issues we have regularly seen in the past. In particular, check that the [Best Practices for RIOT Programming](https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming) are followed. These checks can be aided (but not replaced) by a tool such as Coccinelle, using the script found in dist/tools/coccinelle.
+
+2.1 Check for code duplication  


Section above used punctuation. For consistency I would also prefer it to be used here.

(same goes for section 4 as well of course)

Sorry, just to clarify: do you mean the numbered list format? The paragraphs in sections 2. and 4. indicate a general overview to the section rather than something that would fit in a list format. Perhaps for consistency I could add a short paragraph at the top of section 1.?

I've gone with this, also for section 5.

miri64 · 2018-06-22T19:23:08Z

MAINTAINING.md

+
+### 3. - Test the PR
+
+Run tests to verify the correct behavior (see 1.6), both on Native and on a few selected boards, or present clearly and logically articulated reasons for skipping some/all tests.


`native`

instead of Native?

miri64 · 2018-06-22T19:24:12Z

MAINTAINING.md

+
+### 4. - Review the code against the coding conventions
+
+Check that the code follows the [Coding Conventions](https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions). This can be aided (but not replaced) by Uncrustify, using the uncrustify-riot.cfg file found in the root directory. Note the difference between personal coding style, which is subjective and is allowed subject to the other guidelines, and the coding conventions, which are absolute and must always be followed.


Git-expert nitpick: s/root directory/base directory/ ;-)

miri64

Minor improvement proposal left.

miri64 · 2018-06-26T07:42:51Z

MAINTAINING.md

+
+The following list is not exhaustive and addresses the coding issues we have
+regularly seen in the past. In particular, check that the
+[Best Practices for RIOT Programming](https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming)


The line length problem with URLs could be solved by using references. E.g.

In particular, check that the [Best Practices] for RIOT Programming are followed. ... 2. [Check memory usage][Comparing build sizes] ... [Best Practices]: https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming [Comparing build sizes]: https://github.com/RIOT-OS/RIOT/wiki/Comparing-build-sizes

IMHO that would also make the text more readable in clear text.

miri64

ACK, please squash

@haukepetersen

- This document is the result of original work by @haukepetersen and refinement by @miri64, @aabadie, @kaspar030, @jcarrano, @PeterKietzmann, @ZetaR60, and @jia200x.

danpetry · 2018-06-26T09:13:03Z

Squashed/rebased.

danpetry · 2018-06-26T09:13:17Z

Tests passing.

danpetry added the Area: doc Area: Documentation label Jun 8, 2018

miri64 reviewed Jun 8, 2018

View reviewed changes

danpetry commented Jun 11, 2018

View reviewed changes

PeterKietzmann reviewed Jun 12, 2018

View reviewed changes

jcarrano reviewed Jun 15, 2018

View reviewed changes

danpetry commented Jun 15, 2018

View reviewed changes

danpetry commented Jun 18, 2018

View reviewed changes

aabadie reviewed Jun 22, 2018

View reviewed changes

danpetry commented Jun 22, 2018

View reviewed changes

miri64 requested changes Jun 22, 2018

View reviewed changes

danpetry force-pushed the maintainer_guidelines branch from 287adae to 0ed4ecc Compare June 25, 2018 13:03

miri64 reviewed Jun 26, 2018

View reviewed changes

miri64 approved these changes Jun 26, 2018

View reviewed changes

doc: add MAINTAINING.md

849d209

- This document is the result of original work by @haukepetersen and refinement by @miri64, @aabadie, @kaspar030, @jcarrano, @PeterKietzmann, @ZetaR60, and @jia200x.

danpetry force-pushed the maintainer_guidelines branch from 9a67cec to 849d209 Compare June 26, 2018 08:45

miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 26, 2018

miri64 merged commit 68dc5b0 into RIOT-OS:master Jun 26, 2018

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

danpetry deleted the maintainer_guidelines branch July 12, 2018 14:36


		## 1. - Review the fundaments

		1.1 Does the reasoning for this PR make sense?

		## Detailed Rationales, Explanations, and Examples

		WIP: extend this!

		baseline on things that should be checked.

		Note: Any of the items in this list might be skipped, if good reasons are presented.



		## 3. - Test the PR
		Run tests to verify the correct behavior (see 1.5), or present good reasons

		(1.7 Is the PR a duplicate of another PR?)


		## 2. - Review the code (line-by-line)


		## 4. - Review code style and documentation

		4.1 Check for sufficient high-level (module-level) documentation


		### 1. - Review the fundamentals

		1.1 Does the reasoning for this PR make sense? Are the requirements and design goals clearly thought out and clearly expressed? Is the problem that the PR intends to solve clearly stated?


		The following list is not exhaustive and addresses the coding issues we have regularly seen in the past. In particular, check that the [Best Practices for RIOT Programming](https://github.com/RIOT-OS/RIOT/wiki/Best-Practice-for-RIOT-Programming) are followed. These checks can be aided (but not replaced) by a tool such as Coccinelle, using the script found in dist/tools/coccinelle.

		2.1 Check for code duplication


		### 3. - Test the PR

		Run tests to verify the correct behavior (see 1.6), both on Native and on a few selected boards, or present clearly and logically articulated reasons for skipping some/all tests.


		### 4. - Review the code against the coding conventions

		Check that the code follows the [Coding Conventions](https://github.com/RIOT-OS/RIOT/wiki/Coding-conventions). This can be aided (but not replaced) by Uncrustify, using the uncrustify-riot.cfg file found in the root directory. Note the difference between personal coding style, which is subjective and is allowed subject to the other guidelines, and the coding conventions, which are absolute and must always be followed.

Add maintainer guidelines #9315

Add maintainer guidelines #9315

Conversation

danpetry commented Jun 8, 2018

Contribution description

danpetry commented Jun 8, 2018

miri64 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZetaR60 commented Jun 9, 2018

danpetry left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterKietzmann Jun 12, 2018 • 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

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

danpetry Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpetry commented Jun 11, 2018

ZetaR60 commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpetry Jun 15, 2018 • 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

aabadie commented Jun 15, 2018 • 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

kaspar030 commented Jun 19, 2018

kaspar030 commented Jun 19, 2018

danpetry commented Jun 19, 2018

danpetry commented Jun 19, 2018

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miri64 left a 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

danpetry left a comment •

edited

Loading

PeterKietzmann Jun 12, 2018 •

edited

Loading

danpetry Jun 14, 2018 •

edited

Loading

danpetry Jun 15, 2018 •

edited

Loading

aabadie commented Jun 15, 2018 •

edited

Loading