Page MenuHomePhabricator

Guidelines
Updated 723 Days AgoPublic

This is not meant to be a strict set of rules, but merely a emphasized suggestion, a developer etiquette if you might. This page is for both contributors with commit rights and those without.

Intro

There are many more submitters than reviewers, therefore the only way we can sanely handle patches/commits is not reviewing patches that don't follow the guidelines. Some developers with extra time may review such patches, but it's just better for everyone if the patches just follow the guidelines. The optimal reviewing scenario is: open an email containing a patch, read a nice summary about what the patch does, review the patch to see it's correct, apply, compile, test and if all is well, *copy* the commit message from the email containing the patch. Obviously, if the patch fails to comply to any of the following guidelines, you are encouraged to deny it and request for another patch.

Coding style

The official E coding conventions are at: Ecoding. Patches should *NOT* introduce coding conventions violations. Furthermore, patches should not include whitespace errors (trailing whites, tabs).

Valgrind and Compilation warnings

The patch should not introduce any new warnings with default configure flags and "-Wall -Wextra".
The patch also should not introduce new Valgrind complaints, especially: invalid reads/writes and memory leaks.
Even worse than compilation warnings, is force-silencing the warnings flags, that is, just employing tricks to fool the compiler so it won't complain instead of just fixing the actual issue.

Documentation

All public API functions should be documented using doxygen. You can take a look at Eina/Eet for examples, or just read the doxygen docs. Internal functions and code should also be documented (as much as needed), but this is only a recommendation. Using doxygen for internal functions is recommended, you just need to use @internal to mark it as such.

Git Commit Message

Good commit messages are not hard to write, the idea is to explain the change in a couple of lines in a way that other people (even after long periods of time) will be able to understand.
Preferred log message format:

Lib/app module: Short summary. (length <= 72)

Long summary
More long summary

For example:

Elementary calendar: Take Weekday abbreviated names from locale.

This information is already retrievable from the locale, no need to use
strings and translate them.

If the commit fixes a reported issue then please include the ticket number in the commit message as well

Notable commits

Each commit that fixes a known issue should include, at the end of the message, @fix - so that our ChangeLog parser can hunt them out and report them for release.
Likewise notable features or enhancements should include the symbol @feature so that it can be highlighted in our announcements.

Both of these types of changes use the summary line for context - so remember to make it clear.

More patch guidelines

  1. Patches should not include unrelated changes (fixing both a and b in the same patch, if they can be split, they should be split).
  2. Patches should not include formatting and logic changes in the same patch. For example, fixing indentation and changing a '+' to a '-' in the same patch is prohibited.
  3. Patches should be as small as possible and split among several patch files. It's terribly hard to review huge patches that change thousands of lines.

Sending patches to the developers mailing list

  1. Patches should follow all of the guidelines in this document.
  2. E-mail should include:
    1. The patch (as an attachment, not in the body)
    2. An svn log the reviewer can just copy and paste.
    3. Summary of the patch (what it does, what was the issue and etc.).
    4. A good and descriptive subject line.

Reviewing patches

Feel free to review patches, even if you don't have commit access or you are not a maintainer of the specific piece code. You don't have to be an E-Jedi (master?) to know when a patch includes whitespace errors/compilation warnings or even logic failures. Your review of the patch will speed things up, save time for other people and improve the code quality of E.

Case specific comments

Developers without commit rights

Submit a new bug in phab and send a mail to the mailing list containing a brief explanation and a link to the bug report. If you receive no feedback in the course of 15 days try insisting with another mailing list message.

Warning: mail list strips binary attachments and some clients fail to recognize *.patch and *.diff properly. This is common with GMail and other webmail. Before sending patches, ensure you have the following line in your /etc/mime.types:

text/x-diff  diff patch
Patch file format

Patches are created easily from git, you can simply call git diff > changes.patch to get a patch for changes that have not been staged or git diff --cached > changes.patch if you have staged the changes locally.

Warning: please check Ecoding to know exact coding style before sending patches! It's quite unusual coding style and most people will send patches that will mess it up if our coding style is not followed.

New developers WITH commit rights

Depending on the type of commit you intend to make there are two paths to follow. If you feel confident about your patch, be it because you are a confident person or because the patch fixes a trivial problem, go ahead with your commit. But be warned, developers constantly breaking the build will have their commit rights removed.

If you don't feel confident, probably because your patch implements new functionality or API changes, simply follow the regular path. Submit a new bug in phab and send a mail to the mailing list containing a brief explanation and a link to the bug report. If you receive no feedback in the course of 15 days you are free to commit it without fear.

You have no exempts, you also have to follow the above guidelines completely.

Warning: please check Ecoding to know exact coding style before sending patches! It's quite unusual coding style and most people will send patches that will mess it up if our coding style is not followed.

Add yourself to AUTHORS and doc/{module}.dox.in

We keep no per-file authorship. Instead we use the AUTHORS file in every project.

If your patch contains meaningful code (ie: not just coding style or formatting cleanups), please include yourself to AUTHORS file! If you forget to do so, you can always request it later, but it is better to do it with the patch you're about to send.

To apply AUTHORS changes to the documentation, please include yourself to doc/{module}.dox.in file as well.

Last Author
ajwillia.ms
Last Edited
Oct 25 2017, 7:00 AM
Projects
None
Subscribers
None