vertical alignment of code

Programmers discuss here anything related to FreeOrion programming. Primarily for the developers to discuss.

Moderator: Committer

Post Reply
Message
Author
User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

vertical alignment of code

#1 Post by Dilvish »

**Edit** Looking at the code more, and reading up on some gloss re the python Pep8 style guide more, it's seeming perhaps I thought there was a bigger issue than there really was, and I'm wishing I had gotten it sorted out better before starting the thread. There is still a bit of an issue it would be nice to get consensus on, so I'm going to restate the rest of this post more clearly below (in the fourth post in this thread).

before going too far down a particular path of potential python code cleanup, I wanted to have a bit of a discussion here about it. In the past, FO code both on the C++ side and on the python side has favored vertical alignment of related similar code items. This is at least nominally contrary to the python Pep8 style guide, but the guide does leave some room for discussion.

Although Pep8 clearly recommends against extra spaces for the purpose of vertical code alignment except in a much more narrow group of situations than currently used in the FO project, it also notes "Consistency with this style guide is important. Consistency within a project is more important." (emphasis added) Although the python code style will clearly vary from the C++ code style, I think it is a little relevant that the C++ code side of the project certainly has a strong aspect of favoring vertical alignment of similar adjacent code items. I also note that "Readability counts," and I would join with the people who disagree with Guido about whether or not vertical alignment can improve readability. The strongest thing that in my mind the base Pep8 rule does have in its favor is ease of maintenance -- it's fairly easy to write a regex search and replace to enforce the Pep8 space rule, whereas vertical alignment can easily be thrown off by search and replace renaming and then would require manual oversight to fix. There are admittedly places in the code where it's obvious that an original vertical alignment has degraded due to lack of such oversight. I think I still come down in favor of allowing some vertical alignment, though. Who else has opinions on this?
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: vertical alignment of code

#2 Post by adrian_broher »

Could you please give an example for the 'vertical alignment' we use? I have no idea what you are referring to.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: vertical alignment of code

#3 Post by Geoff the Medio »

I assume it refers to aligning text on adjacent rows so logically similar bits line up, like the function names, parameter and variable names, and comment starts in

Code: Select all

    int                                 turn;                       ///< main game turn
    int                                 system_id;                  ///< ID of system where combat is occurring (could be INVALID_OBJECT_ID ?)
    std::set<int>                       empire_ids;                 ///< IDs of empires involved in combat
    ObjectMap                           objects;                    ///< actual state of objects relevant to combat
    std::map<int, ObjectMap>            empire_known_objects;       ///< each empire's latest known state of objects relevant to combat
    std::set<int>                       damaged_object_ids;         ///< ids of objects damaged during this battle
    std::set<int>                       destroyed_object_ids;       ///< ids of objects destroyed during this battle
    std::map<int, std::set<int> >       destroyed_object_knowers;   ///< indexed by empire ID, the set of ids of objects the empire knows were destroyed during the combat
    Universe::EmpireObjectVisibilityMap empire_object_visibility;   ///< indexed by empire id and object id, the visibility level the empire has of each object.  may be increased during battle
    std::vector<CombatEventPtr>         combat_events;              ///< list of combat attack events that occur in combat

private:
    void    GetEmpireIdsToSerialize(             std::set<int>&                         filtered_empire_ids,                int encoding_empire) const;
    void    GetObjectsToSerialize(               ObjectMap&                             filtered_objects,                   int encoding_empire) const;
    void    GetEmpireKnownObjectsToSerialize(    std::map<int, ObjectMap>&              filtered_empire_known_objects,      int encoding_empire) const;
    void    GetDamagedObjectsToSerialize(        std::set<int>&                         filtered_damaged_objects,           int encoding_empire) const;
    void    GetDestroyedObjectsToSerialize(      std::set<int>&                         filtered_destroyed_objects,         int encoding_empire) const;
    void    GetDestroyedObjectKnowersToSerialize(std::map<int, std::set<int> >&         filtered_destroyed_object_knowers,  int encoding_empire) const;
    void    GetEmpireObjectVisibilityToSerialize(Universe::EmpireObjectVisibilityMap&   filtered_empire_object_visibility,  int encoding_empire) const;
    void    GetCombatEventsToSerialize(          std::vector<CombatEventPtr>&           filtered_combat_events,             int encoding_empire) const;

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: vertical alignment of code

#4 Post by Dilvish »

An example of vertical alignment that would be frowned upon by Pep8 and which is fairly common in the C++ code and which I had mistakenly (I guess too much jumping back and forth between C++ and python) had it in my head that we also do in the python code, is the right hand column in this snippet from TechTreeWnd::TechListBox::TechRow

Code: Select all

    const GG::X GRAPHIC_WIDTH =   col_widths[0];
    const GG::X NAME_WIDTH =      col_widths[1];
    const GG::X COST_WIDTH =      col_widths[2];
    const GG::X TIME_WIDTH =      col_widths[3];
    const GG::X CATEGORY_WIDTH =  col_widths[4];
    const GG::X TYPE_WIDTH =      col_widths[5];
    const GG::X DESC_WIDTH =      col_widths[6];
An example of vertical alignment that we do fair bit on the python side, and which got caught up in some recent proposed automated cleanup is the right hand column in this snippet from AIDependencies.py

Code: Select all

supply_by_size = {int(fo.planetSize.tiny):      2,
                  int(fo.planetSize.small):     1,
                  int(fo.planetSize.large):    -1,
                  int(fo.planetSize.huge):     -2,
                  int(fo.planetSize.gasGiant): -1
                  }
At first I was thinking that this latter example was caught up in the proposed changes because it was discouraged by the Pep8, but after more reading I don't think that's the case. If one of you folks more familiar with Pep8 could confirm that understanding that would be great.

Still, the fact that this python snippet got caught in some automated cleanup for eliminating extra spaces does point out the fact that maintaining such alignments does take some manual work, and so there's a question is it worth it to try to maintain. There are admittedly places in the code where it's obvious that an original vertical alignment has degraded due to lack of sufficient attention, and I've surely contributed to at least some of that in the python code. FO is a long lived project, and ease of style maintenance is a pretty reasonable consideration. I still come out, though, in favor of maintaining such alignments, but I'd like to hear what others have to say on it.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: vertical alignment of code

#5 Post by adrian_broher »

Although Pep8 clearly recommends against extra spaces for the purpose of vertical code alignment except in a much more narrow group of situations than currently used in the FO project, it also notes "Consistency with this style guide is important. Consistency within a project is more important." (emphasis added) Although the python code style will clearly vary from the C++ code style, I think it is a little relevant that the C++ code side of the project certainly has a strong aspect of favoring vertical alignment of similar adjacent code items.
The FreeOrion Python code and C++ code are different domains. Just because they are part of the same project it doesn't mean that the developers need to obey the same style guide for every language, especially if both language have syntax concepts that can't be mapped onto each other.
Dilvish wrote:Still, the fact that this python snippet got caught in some automated cleanup for eliminating extra spaces does point out the fact that maintaining such alignments does take some manual work, and so there's a question is it worth it to try to maintain. There are admittedly places in the code where it's obvious that an original vertical alignment has degraded due to lack of sufficient attention, and I've surely contributed to at least some of that in the python code. FO is a long lived project, and ease of style maintenance is a pretty reasonable consideration. I still come out, though, in favor of maintaining such alignments, but I'd like to hear what others have to say on it.
Well, I consider this vertical alignment styling counterproductive. For having the code look 'nice':
  • you make the source harder to search
  • you make the source harder to substitute
  • you need to edit unrelated lines in a commit for a new feature or bugfix to fix the aligned of those lines
  • you have those dreadful 'grooming' commits because
  • you stumble over the unrelated and 'grooming' commits every time when you search for functional code line changes in the project history
  • you need to merge while rebasing even there is no logical change in code
However changing this now for the sake of change would make everything worse as it would affect the last three points in my list. If you change functionality at the same time I think it's fine.

But I'm generally in favour of using PEP8, as it has one big advantage of everything self made. Python developers are familiar with it, but they aren't familiar with any custom styling. With that you make it easier to contribute to the project for people that know Python. They know how the code should look like and it's easier to read if they familiar with the style. For People who don't know Python it is also easier because PEP8 is a well documented topic, whereas a custom style isn't well documented. Whatever custom style we use just adds another layer of things you need to understand to contribute to the project.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Cjkjvfnby
AI Contributor
Posts: 539
Joined: Tue Jun 24, 2014 9:55 pm

Re: vertical alignment of code

#6 Post by Cjkjvfnby »

Maintance of vertical aligment require additional work.
It does not get huge benefits with readability.

I suggest to follow PEP8.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: vertical alignment of code

#7 Post by Dilvish »

adrian_broher wrote:But I'm generally in favour of using PEP8
Cjkjvfnby wrote:I suggest to follow PEP8.
I started the thread off poorly by thinking the alignment issues I wanted to address were contrary to the advice of Pep8, but as I've indicated above, the alignment cases I'm interested in, such as the alignment of dictionary items above, are not (to my understanding) contrary to Pep8. If you think they are, then please point to a section of Pep8 andexplain why it applies.

Of course, an opinion that any readability benefit from this not worth the extra maintenance hassle can be independent of the issue of it meeting Pep8, but let's be clear here, and not conflate those two things.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
Geoff the Medio
Programming, Design, Admin
Posts: 13587
Joined: Wed Oct 08, 2003 1:33 am
Location: Munich

Re: vertical alignment of code

#8 Post by Geoff the Medio »

adrian_broher wrote:you have those dreadful 'grooming' commits
Grooming commits contain much more than just "vertical alignment" tweaks. And if such commits are objectionable, then all the recent PEP8 conformity commits should also be objectionable, as they are doing mostly the same things conceptually.

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: vertical alignment of code

#9 Post by adrian_broher »

Dilvish wrote:If you think they are, then please point to a section of Pep8 and explain why it applies.
http://legacy.python.org/dev/peps/pep-0008/#pet-peeves

Emphasis added.
More than one space around an assignment (or other) operator to align it with another.

Yes:

Code: Select all

x = 1
y = 2
long_variable = 3
No:

Code: Select all

x             = 1
y             = 2
long_variable = 3
You can see this syntax as assignment of a key to a value within the dict.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
adrian_broher
Programmer
Posts: 1156
Joined: Fri Mar 01, 2013 9:52 am
Location: Germany

Re: vertical alignment of code

#10 Post by adrian_broher »

Geoff the Medio wrote:Grooming commits contain much more than just "vertical alignment" tweaks. And if such commits are objectionable, then all the recent PEP8 conformity commits should also be objectionable, as they are doing mostly the same things conceptually.
I already agree on that, as I already stated in the second to last passage from the post you quoted.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz

User avatar
Dilvish
AI Lead and Programmer Emeritus
Posts: 4768
Joined: Sat Sep 22, 2012 6:25 pm

Re: vertical alignment of code

#11 Post by Dilvish »

adrian_broher wrote:You can see this syntax as assignment of a key to a value within the dict.
Hmm, yes, ok -- it does look like that is the better supported/accepted interpretation regarding dicts.

Ok, I guess that pretty much settles it then, thanks.
If I provided any code, scripts or other content here, it's released under GPL 2.0 and CC-BY-SA 3.0

User avatar
vincele
Space Dragon
Posts: 341
Joined: Sun Mar 23, 2014 6:10 pm

Re: vertical alignment of code

#12 Post by vincele »

adrian_broher wrote:...
I'm with him, even if I never contributed python code to FO, I have some python experience.

My opinion is that python code is not whitespace agnostic, so the only cases that could be argued to be better with vertical alignment are structure initialization (dicts, lists). C++ side is another thing.

Disclaimer: This is not directed against FO patches, this is only my general opinion about such patches.

WRT the grooming commits, I'm not against them, I can find myself doing that kind of things. But I think we must refrain from mixing grooming with anything else, patches must be straightforward. Either grooming or logic change. Only mixing allowed is inside the lines you already change for logic, not other lines "just around them", because it makes patches harder to review. If some grooming is needed around code you change, just make separate patches.
All the patches I'll provide for freeorion will be released under the GPL v2 or later license.
Let's unleash the dyson forest powa!

Post Reply