← Back to team overview

elementary-dev-community team mailing list archive

Re: How to review and merge branches

 

I understand the hesitance to push lovingly crafted code through a
cookie-cutter code beautifier, but we are hardly talking about chopping
everything apart and putting it together backwards.  The only document I
know of (
https://docs.google.com/document/d/1AcuFhcuEkoEM5_JEhGrdeshRQP5oBudh7IPQJeeuxHQ<https://docs.google.com/document/d/1AcuFhcuEkoEM5_JEhGrdeshRQP5oBudh7IPQJeeuxHQ/edit>)
talks about simple rules like whitespace, naming conventions, and the
pre-amble "using" statements.

This stuff can get controversial, but having clean, consistent code between
projects can make it much easier for newer developers like me take a bite
out of the code without curling up into a ball and crying in the corner at
the EOF.

My opinion is that we should strive to add a code formatting phase at the
end of our workflow _before_ requesting a merge to make it easier for the
reviewer.  If it becomes too cumbersome, I don't have any major qualms with
an automated formatting script _as a last resort_.  An automated system
could breed lazy programming practices, or even introduce bugs if not
implemented properly.  However, unification is better than fragmentation.
 It's why we chose a common programming language to begin with, right?

A disciplined programmer is an invaluable programmer, especially when it
comes to mundane details like this.

(please don't look at my personal projects as an example of good code...
nobody is perfect :P)


On Mon, Apr 1, 2013 at 1:10 PM, Victor <victoreduardm@xxxxxxxxx> wrote:

> You're right Craig, although there's something I still don't understand:
> Why would somebody want elementary to adapt his/her coding style.
>
> It's fine if developers focus on the logic first, using their own coding
> style, but as a final step those developers should also make sure that
> their code is consistent with the rest of the code in the project they're
> working on. Shouldn't we as developers review and test our own code before
> proposing a patch anyway? We can always adapt the style of new code during
> that self-review, before making our work available to be reviewed by others.
>
> On Mon, Apr 1, 2013 at 8:51 AM, Craig <weberc2@xxxxxxxxx> wrote:
>
> Personally, I like that I can write code without thinking about the style
> and then have it styled automatically when I push. It lets me focus on the
> logic of my program rather than whether it obeys a style guideline. This is
> especially useful because I participate in projects involving several
> current languages and each with its own style guideline.
>
> I'm not saying we need something like gofmt, but it's foolish to imply
> that such a tool is useless (especially when we are manually investing time
> correcting code that could be done automatically).
>
> If an appropriate tool doesn't exist, I don't recommend developing one,
> but I don't see how you can mock gofmt when I can validate my style with no
> overhead whatsoever while you are doing it manually. Lol. ;-)
> On Apr 1, 2013 9:28 AM, "David Gomes" <david@xxxxxxxxxxxxxxxx> wrote:
>
>> Fortunately, most of the developers can write good code. And when they
>> fail to do so we have other developers who review their code.
>>
>> We don't need a fancy tool like gofmt that just changes our code.
>>
>>
>> On Mon, Apr 1, 2013 at 3:25 PM, Craig <weberc2@xxxxxxxxx> wrote:
>>
>>> The more I read threads like this the more it seems elementary should
>>> migrate to Go. :-P
>>> On Apr 1, 2013 3:29 AM, "Jaap Broekhuizen" <jaapz.b@xxxxxxxxx> wrote:
>>>
>>>> I agree with Victor. Consistency matters because it makes readability
>>>> and therefore maintainability better.
>>>>
>>>> --
>>>> Jaap
>>>> Op 1 apr. 2013 09:09 schreef "Victor" <victoreduardm@xxxxxxxxx> het
>>>> volgende:
>>>>
>>>>> Coding style is a subjective topic, and that's why discussing which
>>>>> one works best is completely pointless, since it's a matter of preferences.
>>>>> It's like discussing what is the best color.
>>>>>
>>>>> What is important is consistency, and that's why all the new code
>>>>> proposed for merging should follow elementary's coding style guidelines
>>>>> (which are not published anywhere in the site as far as I know). Whenever
>>>>> you propose code that is styled inconsistently it only gives the impression
>>>>> that you were coding in a hurry, and we don't want to accept that kind of
>>>>> code, even though we have a ton of it already.
>>>>>
>>>>> Thanks for your attention,
>>>>> Victor.
>>>>>
>>>>> On Sun, Mar 31, 2013 at 12:48 PM, Craig <weberc2@xxxxxxxxx> wrote:
>>>>>
>>>>> How do you figure? The go language community uses one and they rave
>>>>> about it. We use them at work (c++) as well and its uses an obnoxious
>>>>> style, but it's still more readable than a dozen different conventions.
>>>>> On Mar 31, 2013 5:39 AM, "Sergey "Shnatsel" Davidoff" <
>>>>> sergey@xxxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>>> I'm afraid automatic "prettifiers" are a terrible idea because
>>>>>> blindly restyling the code usually makes it lose any remains of readability
>>>>>> it used to have. In other words, automatically restyled code is even less
>>>>>> readable than code with a foreign coding style.
>>>>>>
>>>>>>
>>>>>> 2013/3/31 David Gomes <david@xxxxxxxxxxxxxxxx>
>>>>>>
>>>>>>> I wrote this in order to check for code style errors, but it's not
>>>>>>> perfect it's just a help-tool:
>>>>>>>
>>>>>>> https://github.com/elementary/vala-analyzer
>>>>>>>
>>>>>>> We have 'considered' using a prettifier too, but I just use Emacs to
>>>>>>> fix some stuff on my code - a prettifier script would be too much work and
>>>>>>> I don't know of any libraries that would help me with the task.
>>>>>>>
>>>>>>>
>>>>>>> On Sun, Mar 31, 2013 at 3:34 AM, Craig <weberc2@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>>> Good work David. Have you (elementary) considered using a
>>>>>>>> prettifier to standardize a code style upon pushing to your trunk?
>>>>>>>>  On Mar 28, 2013 7:17 PM, "Cody Garver" <cody@xxxxxxxxxxxxxxxx>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Cool, it's pretty thorough.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, Mar 27, 2013 at 7:58 AM, David Gomes <
>>>>>>>>> david@xxxxxxxxxxxxxxxx> wrote:
>>>>>>>>>
>>>>>>>>>> http://dl.dropbox.com/u/19899464/reviewstutorial.html
>>>>>>>>>>
>>>>>>>>>> Hello guys,
>>>>>>>>>>
>>>>>>>>>> From time to time somebody still has doubts on how to use
>>>>>>>>>> Launchpad and Bazaar to review and merge branches to trunk so I wrote a
>>>>>>>>>> tutorial. Note though that it may need expansion.
>>>>>>>>>>
>>>>>>>>>> Many times, even experienced developers who have been in the Apps
>>>>>>>>>> Team for a long time make mistakes so even if you already know how to do
>>>>>>>>>> it, reading the tutorial won't hurt.
>>>>>>>>>>
>>>>>>>>>> I also recommend that all developers that in the future are to
>>>>>>>>>> join the Apps Team read this several times because even though we can
>>>>>>>>>> always revert messed-up commits, it's better to do it right at the first
>>>>>>>>>> time.
>>>>>>>>>>
>>>>>>>>>> Best regards,
>>>>>>>>>> David "Munchor" Gomes
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Mailing list: https://launchpad.net/~elementary-dev-community
>>>>>>>>>> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
>>>>>>>>>> Unsubscribe : https://launchpad.net/~elementary-dev-community
>>>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Cody Garver
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Mailing list: https://launchpad.net/~elementary-dev-community
>>>>>>>>> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
>>>>>>>>> Unsubscribe : https://launchpad.net/~elementary-dev-community
>>>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Mailing list: https://launchpad.net/~elementary-dev-community
>>>>>>> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
>>>>>>> Unsubscribe : https://launchpad.net/~elementary-dev-community
>>>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Sergey "Shnatsel" Davidoff
>>>>>> OS architect @ elementary
>>>>>>
>>>>>
>>>>> --
>>>>> Mailing list: https://launchpad.net/~elementary-dev-community
>>>>> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
>>>>> Unsubscribe : https://launchpad.net/~elementary-dev-community
>>>>> More help   : https://help.launchpad.net/ListHelp
>>>>>
>>>>>
>>> --
>>> Mailing list: https://launchpad.net/~elementary-dev-community
>>> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
>>> Unsubscribe : https://launchpad.net/~elementary-dev-community
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>>
>>
> --
> Mailing list: https://launchpad.net/~elementary-dev-community
> Post to     : elementary-dev-community@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~elementary-dev-community
> More help   : https://help.launchpad.net/ListHelp
>
>

Follow ups

References