[style] Maximum line length #1785

odow · 2019-01-16T14:34:13Z

This one is going to be contentious (looking at @chriscoey).

Ref issue: #1780
Ref previous PR: #1781

codecov · 2019-01-16T16:01:32Z

Codecov Report

Merging #1785 into master will increase coverage by 13.61%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1785       +/-   ##
===========================================
+ Coverage   69.57%   83.18%   +13.61%     
===========================================
  Files          30       41       +11     
  Lines        3957     6270     +2313     
===========================================
+ Hits         2753     5216     +2463     
+ Misses       1204     1054      -150

Impacted Files	Coverage Δ
src/shapes.jl	`40% <0%> (ø)`
src/_Derivatives/conversion.jl	`98.07% <0%> (ø)`
src/_Derivatives/linearity.jl	`97.82% <0%> (ø)`
src/_Derivatives/sparsity.jl	`97.46% <0%> (ø)`
src/_Derivatives/types.jl	`100% <0%> (ø)`
src/_Derivatives/_Derivatives.jl	`100% <0%> (ø)`
src/_Derivatives/reverse.jl	`93.47% <0%> (ø)`
src/_Derivatives/coloring.jl	`97.97% <0%> (ø)`
src/_Derivatives/topological_sort.jl	`100% <0%> (ø)`
src/_Derivatives/forward.jl	`97.61% <0%> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 350e948...fbf94e2. Read the comment docs.

chriscoey · 2019-01-16T22:37:58Z

IMO harder to read and archaic given modern code viewers and editors (wrapping), ~~but I won't put up a fight~~.

mlubin · 2019-01-16T22:48:45Z

docs/src/style.md


-#### TODO: Line breaks
+Following the Google style guide for [line length](https://github.com/google/styleguide/blob/gh-pages/pyguide.md#32-line-length)
+in python, keep lines under an 80 character limit.


This is a contentious point so we need to give our own motivation and not just defer to the google style guide.
My thinking is that for consistency, we need some guideline on line length. 80 characters is a reasonable pre-existing convention.

Maybe we just make this a recommendation. What about:

Our foremost goal is to maximize code readability. Very long line lengths are hard to read since they either require horizontal scrolling, or are wrapped uncontrollably by a text editor. Thus, where possible, keep line lengths under 80 characters (an arbitrary, but pre-existing convention). We make an exception for lines in which manually breaking the line over multiple lines is worse for readability.

Examples include

URLs

path names

long string constants not containing whitespace

"wrapped uncontrollably by a text editor" is pretty unconvincing

yes, please just make it a recommendation. I don't plan to start enforcing line lengths in any packages I author. though for packages that aren't mine, I will follow the surrounding style.

@mlubin it is also "consistent" to not have line limits at all

joaquimg · 2019-01-17T01:14:47Z

Maybe we should consider that we use verbose names for everything. We also have many parametric types. And 4 spaces tabs...
I don't like that limit.
I would like to vote for a larger limit at least.
Why 80? wasn't that chosen in the 1930's with no widescreens?

chriscoey · 2019-01-17T05:12:56Z

@joaquimg makes a great point I hadn't even thought of.

though I prefer no line limit, I would be OK with a "target/soft maximum line length" of n characters (n = 120 maybe?). if the limit is arbitrary anyway, we may as well be a little soft about it. then n chars is a goal, and clearly a line of 1.5*n chars is too long and should in good conscience be broken up. guidelines like "verbose variable names" require judgment and subjectivity, so why not line lengths too?

the code is littered with ugliness like https://github.com/JuliaOpt/MathOptInterface.jl/blob/f4db9518c49a85b70f72043f01a2bce93d44d981/src/Test/contlinear.jl#L1482 where with a target line length of 80 chars we would be allowed to just keep it on the same line of ~85 chars.

chriscoey · 2019-01-17T05:23:46Z

i want to avoid feeling as though the style guide is too totalitarian. it's not cute. i see no reason to perpetuate old python style doctrines.

finally, i will make another plug for recommending semantic line breaking for long comments and print messages (and the same idea can be applied when breaking up long lines of code into coherent chunks).

blegat · 2019-01-17T10:26:47Z

Even on modern screens, it still makes sense to use 80 characters because we might split our screen to edit multiple files at the same time and 80 characters is better for readability.
Note though that PEP-8 allows to use 100 as limit (https://www.python.org/dev/peps/pep-0008/#maximum-line-length)

Some teams strongly prefer a longer line length. For code maintained exclusively or primarily by a team that can reach agreement on this issue, it is okay to increase the nominal line length from 80 to 100 characters (effectively increasing the maximum length to 99 characters), provided that comments and docstrings are still wrapped at 72 characters.

By the way, for this reason, this is the Python limit recorded by wikipedia

odow · 2019-01-17T14:43:32Z

the code is littered with ugliness like

    obj = MOI.ScalarAffineFunction(MOI.ScalarAffineTerm.([2.0, 1.0], [x, y]),
                                   0.0)

True, that's not very nice. A better way (to my eyes) is:

    obj = MOI.ScalarAffineFunction(
        MOI.ScalarAffineTerm.([2.0, 1.0], [x, y]), 
        0.0)

Breaking things up into smaller lines makes each statement easier to read. Consider:

    obj = MOI.ScalarAffineFunction(MOI.ScalarAffineTerm.([2.0, 1.0], [x, y]), 0.0)
    obj = MOI.ScalarAffineFunction(MOI.ScalarAffineTerm.([2.0, 1.0], [x, y], 0.0))

mlubin · 2019-01-17T23:17:50Z

@chriscoey the reference on semantic line breaks is specifically for text. What do semantic line breaks mean for code?

chriscoey · 2019-01-18T01:39:35Z

the code is littered with ugliness like

    obj = MOI.ScalarAffineFunction(MOI.ScalarAffineTerm.([2.0, 1.0], [x, y]),
                                   0.0)

True, that's not very nice. A better way (to my eyes) is:

    obj = MOI.ScalarAffineFunction(
        MOI.ScalarAffineTerm.([2.0, 1.0], [x, y]), 
        0.0)

agreed with @odow, that looks nicer. so why don't we recommend how to break up lines, so that this outcome results? in particular, 4 spaces indenting only.

all throughout the code we have things like:

function Base.copy(set::Union{Reals, Zeros, Nonnegatives, Nonpositives,
                              GreaterThan, LessThan, EqualTo, Interval,
                              SecondOrderCone, RotatedSecondOrderCone,
                              GeometricMeanCone, ExponentialCone,
                              DualExponentialCone, PowerCone, DualPowerCone,
                              PositiveSemidefiniteConeTriangle,
                              PositiveSemidefiniteConeSquare,
                              LogDetConeTriangle, LogDetConeSquare,
                              RootDetConeTriangle, RootDetConeSquare,
                              Integer, ZeroOne, Semicontinuous, Semiinteger})
    return set
end

whereas it would be more consistent to use 4 spaces indenting and write

function Base.copy(set::Union{
    Reals, Zeros, Nonnegatives, Nonpositives,
    GreaterThan, LessThan, EqualTo, Interval,
    SecondOrderCone, RotatedSecondOrderCone,
    GeometricMeanCone, ExponentialCone,
    DualExponentialCone, PowerCone, DualPowerCone,
    PositiveSemidefiniteConeTriangle,
    PositiveSemidefiniteConeSquare,
    LogDetConeTriangle, LogDetConeSquare,
    RootDetConeTriangle, RootDetConeSquare,
    Integer, ZeroOne, Semicontinuous, Semiinteger
    })
    return set
end

I'm fine with soft line length limits if it is accompanied by consistent indenting practices. though i agree with @joaquimg and think 80 chars is too small given that variable and function names etc have to be verbose now.

mlubin · 2019-01-18T21:26:48Z

so why don't we recommend how to break up lines, so that this outcome results?

That's a good point. We don't do this consistently now. I would look to a few code autoformatting tools autopep8, gofmt, and clang-format for how they align function calls and other lists of things that overflow a line.

joaquimg · 2019-01-19T18:48:49Z

Recommendations on how to break lines would be great!
Softer limit in 80 might be good also, allowing to go to 100 sometimes. (Of course I’d like more columns ;D )

docs/src/style.md

mlubin

I've been putting off looking at this because I don't enjoy debating about line lengths, but the suggestions are pretty reasonable. If we cite an existing autoformatter for people to experiment with if they're unsure then I think we're covered.

docs/src/style.md

odow · 2019-04-08T14:56:35Z

Okay, I've removed the contentious points. I'm in favour of merging this so we can move past the discussion.

docs/src/style.md

mlubin · 2019-04-09T01:49:00Z

@joaquimg @blegat PTAL.

@chriscoey Your major objection of not having providing guidelines for how to break a line is now mostly addressed. https://yapf.now.sh/ seems to provide a reasonable point of reference to the extent that julia syntax overlaps with python syntax. What do you think?

joaquimg · 2019-04-09T02:48:43Z

Looks good.

chriscoey · 2019-04-09T03:13:37Z

looks good.

aside: would be nice to have a Julia version of YAPF

odow · 2019-04-09T15:50:26Z

Merged. Anyone wanting to re-ignite the debate can make a new PR.

[style] Maximum line length

038dea7

odow force-pushed the od/line_length branch from a903b07 to 038dea7 Compare January 16, 2019 14:35

odow mentioned this pull request Jan 16, 2019

Missing style guide points #1780

Closed

40 tasks

blegat approved these changes Jan 16, 2019

View reviewed changes

mlubin reviewed Jan 16, 2019

View reviewed changes

Add relaxed line length and examples for line breaks

1a880ad

odow mentioned this pull request Jan 25, 2019

Add style guide section on line breaks #1786

Closed

Remove cruft

018b760

blegat reviewed Jan 25, 2019

View reviewed changes

docs/src/style.md Outdated Show resolved Hide resolved

blegat reviewed Jan 25, 2019

View reviewed changes

docs/src/style.md Show resolved Hide resolved

blegat reviewed Jan 25, 2019

View reviewed changes

docs/src/style.md Outdated Show resolved Hide resolved

blegat reviewed Jan 25, 2019

View reviewed changes

docs/src/style.md Outdated Show resolved Hide resolved

odow added 2 commits January 25, 2019 08:30

Updates to line breaking recommendations

f919d6f

Add line length exceptions

96f10c2

mlubin reviewed Feb 27, 2019

View reviewed changes

docs/src/style.md Outdated Show resolved Hide resolved

docs/src/style.md Outdated Show resolved Hide resolved

docs/src/style.md Outdated Show resolved Hide resolved

docs/src/style.md Outdated Show resolved Hide resolved

odow added 2 commits April 8, 2019 09:50

Remove contentious suggestions

e140504

Add YAPF

d17303c

mlubin reviewed Apr 9, 2019

View reviewed changes

docs/src/style.md Outdated Show resolved Hide resolved

Update style.md

fbf94e2

blegat approved these changes Apr 9, 2019

View reviewed changes

odow merged commit 903821b into master Apr 9, 2019

odow deleted the od/line_length branch April 9, 2019 15:50

blegat mentioned this pull request Mar 13, 2023

bare minimum access to complex interface jump-dev/SeDuMi.jl#24

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[style] Maximum line length #1785

[style] Maximum line length #1785

odow commented Jan 16, 2019

codecov bot commented Jan 16, 2019 •

edited

Loading

chriscoey commented Jan 16, 2019 •

edited

Loading

mlubin Jan 16, 2019

odow Jan 16, 2019 •

edited

Loading

chriscoey Jan 16, 2019

chriscoey Jan 16, 2019

chriscoey Jan 17, 2019

joaquimg commented Jan 17, 2019

chriscoey commented Jan 17, 2019

chriscoey commented Jan 17, 2019 •

edited

Loading

blegat commented Jan 17, 2019 •

edited

Loading

odow commented Jan 17, 2019

mlubin commented Jan 17, 2019

chriscoey commented Jan 18, 2019 •

edited

Loading

mlubin commented Jan 18, 2019

joaquimg commented Jan 19, 2019

mlubin left a comment

odow commented Apr 8, 2019

mlubin commented Apr 9, 2019

joaquimg commented Apr 9, 2019

chriscoey commented Apr 9, 2019

odow commented Apr 9, 2019

[style] Maximum line length #1785

[style] Maximum line length #1785

Conversation

odow commented Jan 16, 2019

codecov bot commented Jan 16, 2019 • edited Loading

Codecov Report

chriscoey commented Jan 16, 2019 • edited Loading

mlubin Jan 16, 2019

Choose a reason for hiding this comment

odow Jan 16, 2019 • edited Loading

Choose a reason for hiding this comment

chriscoey Jan 16, 2019

Choose a reason for hiding this comment

chriscoey Jan 16, 2019

Choose a reason for hiding this comment

chriscoey Jan 17, 2019

Choose a reason for hiding this comment

joaquimg commented Jan 17, 2019

chriscoey commented Jan 17, 2019

chriscoey commented Jan 17, 2019 • edited Loading

blegat commented Jan 17, 2019 • edited Loading

odow commented Jan 17, 2019

mlubin commented Jan 17, 2019

chriscoey commented Jan 18, 2019 • edited Loading

mlubin commented Jan 18, 2019

joaquimg commented Jan 19, 2019

mlubin left a comment

Choose a reason for hiding this comment

odow commented Apr 8, 2019

mlubin commented Apr 9, 2019

joaquimg commented Apr 9, 2019

chriscoey commented Apr 9, 2019

odow commented Apr 9, 2019

codecov bot commented Jan 16, 2019 •

edited

Loading

chriscoey commented Jan 16, 2019 •

edited

Loading

odow Jan 16, 2019 •

edited

Loading

chriscoey commented Jan 17, 2019 •

edited

Loading

blegat commented Jan 17, 2019 •

edited

Loading

chriscoey commented Jan 18, 2019 •

edited

Loading