vertical alignment of code
Moderator: Committer
vertical alignment of code
**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?
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
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: vertical alignment of code
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
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
- Geoff the Medio
- Programming, Design, Admin
- Posts: 13587
- Joined: Wed Oct 08, 2003 1:33 am
- Location: Munich
Re: vertical alignment of code
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;
Re: vertical alignment of code
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
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
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.
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];
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
}
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
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: vertical alignment of code
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.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.
Well, I consider this vertical alignment styling counterproductive. For having the code look 'nice':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.
- 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
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
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
Re: vertical alignment of code
Maintance of vertical aligment require additional work.
It does not get huge benefits with readability.
I suggest to follow PEP8.
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
Re: vertical alignment of code
adrian_broher wrote:But I'm generally in favour of using 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.Cjkjvfnby wrote:I suggest to follow PEP8.
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
- Geoff the Medio
- Programming, Design, Admin
- Posts: 13587
- Joined: Wed Oct 08, 2003 1:33 am
- Location: Munich
Re: vertical alignment of code
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.adrian_broher wrote:you have those dreadful 'grooming' commits
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: vertical alignment of code
http://legacy.python.org/dev/peps/pep-0008/#pet-peevesDilvish wrote:If you think they are, then please point to a section of Pep8 and explain why it applies.
Emphasis added.
You can see this syntax as assignment of a key to a value within the dict.More than one space around an assignment (or other) operator to align it with another.
Yes:
No:Code: Select all
x = 1 y = 2 long_variable = 3
Code: Select all
x = 1 y = 2 long_variable = 3
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
- adrian_broher
- Programmer
- Posts: 1156
- Joined: Fri Mar 01, 2013 9:52 am
- Location: Germany
Re: vertical alignment of code
I already agree on that, as I already stated in the second to last passage from the post you quoted.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.
Resident code gremlin
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
Attached patches are released under GPL 2.0 or later.
Git author: Marcel Metz
Re: vertical alignment of code
Hmm, yes, ok -- it does look like that is the better supported/accepted interpretation regarding dicts.adrian_broher wrote:You can see this syntax as assignment of a key to a value within the dict.
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
Re: vertical alignment of code
I'm with him, even if I never contributed python code to FO, I have some python experience.adrian_broher wrote:...
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!
Let's unleash the dyson forest powa!