Solve Style Annoyances

[jgkamat]

Currently, checkstyle does a bunch of annoying things, such as inlining one line functions in c++.

Please comment on other things you might want to change. I am in favor of bumping the line limit to 120 chars as well (I think the current line limit is way too short given widescreen monitors.)

Let's first decide what we wan't (thats the hard part) and then we can figure out how to do it in clang-format.

PROPOSED CHANGES:

• [x] Disallow inlining of functions in c++ - @jgkamat

- [ ] Bump line limit to 120 chars - @jgkamat

Continued from RoboJackets/robocup-firmware#24

[jgkamat]

Please add suggestions by commenting or editing the list, we'll check off these items if they have enough approval.

[jpfeltracco]

:-1: On line limit change. 80 chars is perfect for side by side editing files which I depend upon.

[jgkamat]

https://retracile.net/wiki/VimBreakIndent

While 80 characters is ok for side by side editing, it makes full screen editing super painful (while long lines can easily be wrapped, short lines cannot be dynamically unwrapped without something super complex).

This is what I see when working on something wrapped at about 80 llines...

Personally, I think that we should at least increase it to 100 lines (making it way easier to wrap long lines, as alignment won't be such a pain for ultra long lines). We'll discuss this in the next meeting.

[ashaw596]

I'd support a 120 line limit! 80 makes many of our function calls so hard to parse.

On Sep 29, 2016 4:12 PM, "Jay Kamat" notifications@github.com wrote:

https://retracile.net/wiki/VimBreakIndent

While 80 characters is ok for side by side editing, it makes full screen editing super painful (while long lines can be wrapped, short lines cannot be dynamically unwrapped without something super complex).

This is what I see when working on something wrapped at about 80 llines...

Personally, I think that we should at least increase it to 100 lines (making it way easier to wrap long lines, as alignment won't be such a pain for ultra long lines). We'll discuss this in the next meeting.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[jpfeltracco]

@ashaw596 What do you mean by "hard to parse?"

[jgkamat]

Consider this example: https://github.com/RoboJackets/robocup-software/blob/master/soccer/gameplay/tactics/one_touch_pass.py#L55. I broke the rules with this one time and that line is 120 chars long

Here is my (trying to) format this to 80 chars. I've preserved the indentation in the file at that point (since python forces indentation)

        if self.force_reevauation == True \
           or pass_bhvr.receive_point == None \
           or pass_bhvr.target_point == None \
           or probability > OneTouchPass.tpass.eval_single_point(main.ball().pos,
                                                                 pass_bhvr.receive_point, ignore_robots=pass_bhvr.get_robots()) \
                                                                 + OneTouchPass.receivePointChangeThreshold:

This starts off great, with the first few function args handling well, but then we get to: or probability > OneTouchPass.tpass.eval_single_point(main.ball().pos, which on its own is 71 chars long. Unfortunatley, with indentation, the char before the comma is at 80 chars, so we 'have to' indent this. Indenting before the > looks weird (an early indent) while indenting the OneTouchPass.tpass would cause confusion and is undesirable, since OneTouchPass.something is one statement (imo, these should always be on the same line). This leaves us with only the option to indent after the opening paren, which forces us to align our function call, as well as indent. Every line after that one has only 16 chars to live, which is pretty bad. This problem can also be reached if we nest a bunch of python scopes, and there's no way around it.

The 80 char limit was also used when TABS were widely used. Us using 4 spaces for indentation puts a wrench in this, since it gives us 4 less chars to work with with every indentation level.

Once we are in one of these situations, it's impossible to keep the 80 line limit. Basically, there's not a good solution to this, other than to make the line limit longer or to make a one off exception. IMO making the line limit ever so slightly longer makes this way better, since there are a lot of lines close to 80 chars long that could be broken up after a paren, so we could avoid the alignment issue. More talk on sunday though... :smile:

[jpfeltracco]

• Tabs are still widely used in the linux kernel and they are defined as being 8 chars long so I'm not sure I really understand your point. This, along with the limit of 80 chars forces them to limit the amount of nesting they do, breaking things into separate functions as needed. IMO this probably isn't a bad thing and 80 chars clearly doesn't hamper larger software projects too much. In addition, PEP8 (the reference Python style-guide) further limits line length down to 79 characters (although they mention if the whole team is onboard, moving up to 80-100 characters is okay if docstrings/comments are limited to 72. They also suggest 4 space indents.
• Python indentation is actually quite flexible once you realize that anything in parens is assumed to be continued over lines. I would probably try to break that OneTouchPass out, but if that weren't possible, my initial attempt would likely be something like this (I think the "is" operator is preferable to the equality operator for None checks in Python but check PEP8):

        if (self.force_reevauation
            or pass_bhvr.receive_point is None
            or pass_bhvr.target_point is None
            or probability > (
                OneTouchPass.tpass.eval_single_point(
                    main.ball().pos,
                    pass_bhvr.receive_point,
                    ignore_robots=pass_bhvr.get_robots())
                + OneTouchPass.receivePointChangeThreshold)):

[jgkamat]

The reason why I mentioned tabs before is that every developer can change the size of their tabstops (which allows them to get more stuff on a single line, solving the problem). With this, developers with a small screen or splitting windows can set their tabstop size to 1, which gives more room. If you were working on a linux kernel with a ~100 line len with tabs, you could set the tabstop size to 1, and you could get heavily nested functions to stay on your screen. Long lines on the base would still get you though, but most issues with running over the limit happen at deep indentation levels anyway.

The is operator is definetly preferred, I'll change that next time I work on the onetouchpass again.

I personally really hate breaking open parenthasis without at least a single line, but it's definetly a way to work around this issue. This is still something that comes up if we start to use more descriptive names. Also while I trust you or another member to properly format everything, clang format would probably make a ton of splits in random places, which only makes everything harder to understand. I definetly don't want to be thinking about style when I'm programming, so all of my code will be formatted with the formatter. This is the formatter's result on that line (yes, it's configured for 80 chars)

        if self.force_reevauation == True or pass_bhvr.receive_point == None or pass_bhvr.target_point == None or probability > OneTouchPass.tpass.eval_single_point(
                main.ball().pos,
                pass_bhvr.receive_point,
                ignore_robots=pass_bhvr.get_robots(
                )) + OneTouchPass.receivePointChangeThreshold:

and this really bugs me, because the logs say I wrote this.

In the end I just want a system that works (without me thinking about it). Ideally, we would all ignore line limits and soft word wrap would be perfect, but there's no good soft word wrap for every text editor that includes alignment.

Probably in the end we'll stick with 80 chars, but we'll see what people think in the meeting.

[jpfeltracco]

I've been browsing through our python code a bit and the fact that we need to indent twice (class level then function level) mixed with the fact that we have (fairly lengthy) function names that have to be preceeded with a self makes us already lose 16 columns before any real code has been written. Then we refer to fully qualified names which totally kills the rest of the space (behavior.Behavior.State is 23 columns in and of itself). Plus, the formatter won't add parens to break up long statements and instead just goes with a longer line width anyway forcing the programmer to handle it. So I agree with you that for our current python code base, 80 columns isn't working and we need to look into solutions.

So, I think to find a solution we may want to....

1. see whether or not we can shorten variable and function names and/or use the from <module> import <something> syntax to help us stick to 80 cols without sacrificing clarity. (I'm not sure this is possible)
2. rewrite a play to leverage 120 cols and see if the result is more or less readable after passing it through the code formatter.
3. investigate ways to make the checkstyle more permissive. Checkstyle should make our code base better, not worse. I completely agree that what the autoformatter did to your code is kind of an atrocity and it should have stuck to however you originally styled it. Humans are usually better judges than computers for what is readable :smile:

Looking forward to discussing Sunday, I think getting this sorted out will make things much nicer for freshmen that would like to get into plays and will need to read this stuff.

[jgkamat]

One option for fixing the long classnames is using the 'advanced?' import (I'm honestly not sure if what I'm saying is correct, since I guess and check with these but here goes):

Example: tactics.coordinated_pass.CoordinatedPass.State.receiving

from tactics.coordinated_pass import CoordinatedPass as Pass
Pass.State.receiving

Which (IMO) is way more readable. This only applies to python though, I don't know if we can do something like this in c++.

I'm not sure if this would confuse or help new members though... If I didn't know anything about the from import, it would (and has in the past) confused me. We could make it part of python training.

[jpfeltracco]

Yeah, I like import aliasing (my made up term for it), and I think we should use it liberally, but we have to stay consistent about it through our python code base or I could see it getting a little annoying.

[justbuchanan]

Unfortunately the imports of behaviors, etc have have to be full length in order for the auto-reloading system to work.

If you do something like from skills.move import Move, it essentially does import skills.move; Move = skills.move.Move. Then later if you modify move.py and it gets auto-reloaded, your version of Move will be the old one. If you use the long-form import, everything works as expected because it's grabbing the current version of the module/class from the system.

It's unfortunate that it works this way since it makes our code pretty verbose, but I'm not sure there's a good way around it. If you can find one, go for it!

For reference, the auto-reloading code is here. Also, there's a note about the import syntax and auto-reloading here.

[joshhting]

To eliminate any confusion among the new members with whatever style changes we decide to implement, whether it be import aliasing or something else, we could have it as part of their git commit training. Have each member pick a different python file and have everyone apply and commit these style changes for their file.

[jgkamat]

@justbuchanan I've been having a few errors with the play reloading system (I think it's something weird with the way my editor saves, as talked about in the readme), so I'll look into messing with the play reloading system. #803

AFAIK, I think that the from x import y as z reloading will work as long as y is a python module (which means it won't work for the example above).

This, however, works:

import tactics.coordinated_pass as cp

Which isn't as useful, but could be nice to use. Using them at all makes it more likeley for people to use them without knowing what's going on and break the play reloading system though.

[justbuchanan]

Good find! I didn't realize that the import __ as __ syntax worked with reloading. Being able to omit the long long import names throughout the plays and behaviors should help a good bit.

[jgkamat]

We agreed at the meeting that we should stick to 80 chars for now, but mess with the formatter to stop it from messing with our code. For python, this is a good starting point: SPLIT_BEFORE_LOGICAL_OPERATOR

[jgkamat]

We will disallow inlining of functions in cpp files but continue to allow it in header files if the implementation is defined in the header file.

[jgkamat]

I spent a bunch of time staring at the config options for both clang and yapf and they don't seem to help too much (regarding the splitting of long lines). I tried a bunch of settings but they don't seem to do anything. For pyhton, it does help to put () around long lines so yapf has more options, but I can't find anything to do the same for C++.

As for inlining functions, I think that setting AllowShortFunctionsOnASingleLine to SFS_Inline (which only merges functions in a class) would be good, but it's too liberal. Any thoughts on that?

clang format docs: http://clang.llvm.org/docs/ClangFormatStyleOptions.html yapf docs: https://github.com/google/yapf

We might also want to consider switching to astyle or uncrustify for c, and maybe turning off style formatting altogether for python (so we would manually fix style errors).

[jgkamat]

IMO this is mostly fixed now. Please reopen this issue if you find anything that stands out pretty bad.