BlueStyle icon indicating copy to clipboard operation
BlueStyle copied to clipboard

Exempting literal strings from line limits?

Open nickrobinson251 opened this issue 4 years ago • 13 comments

It is not uncommon to want verbose logging messages (in the internal Invenia codebase, and Production systems more generally).

If there are multiple log statements, it is easy to end up with 5-10 lines dedicated some form of logging messages. But once a function is over ~30 lines it becomes hard to read, just because of the amount of vertical space used. So for log messages, the horizontal space limit puts notable pressure on the vertical space.

One option to address this would be to exempt literal strings from line limits, then codebases can choose to allow log messages that are greater than 92 chars, without incurring lots of vertical space. e.g. to allow

function do_thing(status)
    is_good(status) || warn(LOGGER, "Whoa there. Things aren't all good. Doing the thing may lead to some seriously weird stuff happening.")
    return the_thing()
end

rather than things like this:

function do_thing(status)
    is_good(status) || warn(
        LOGGER,
        "Whoa there. Things aren't all good. " *
        "Doing the thing may lead to some seriously weird stuff happening."
    )
    return the_thing()
end
function do_thing(status)
    if !is_good(status)
        warn(
            LOGGER,
            string(
                "Whoa there. Things aren't all good. ",
                "Doing the thing may lead to some seriously weird stuff happening."
            )
        )
    end
    return the_thing()
end

nickrobinson251 avatar Aug 27 '20 14:08 nickrobinson251

Hmm I think there are two exemptions here, and one is easier to define than the other.

One:

function do_thing(status)
    is_good(status) || warn(LOGGER, 
        "Whoa there. Things aren't all good. Doing the thing may lead to some seriously weird stuff happening."
    )
    return the_thing()
end

Here the exception applies when a line ends in a literal string.

Two:

function do_thing(status)
    is_good(status) || warn(LOGGER, "Whoa there. Things aren't all good. Doing the thing may lead to some seriously weird stuff happening.")
    return the_thing()
end

Here the exemption applies when the line ends in a literal string and some insignificant closing content which is implied by visible content (i.e., the ) is expected because we have a dangling ( in the visible area).

The first is easy to define; how do we define the second?

iamed2 avatar Aug 27 '20 14:08 iamed2

the line ends in a literal string and some insignificant closing content

can we just name ) and , as okay "closing content"? are there other candidates?

nickrobinson251 avatar Aug 27 '20 14:08 nickrobinson251

;, }, ], any other custom brackets, end, some operator like *, some operator like ||, maybe more

iamed2 avatar Aug 27 '20 14:08 iamed2

If there's going to be some documentation on what's good and bad "closing content", there's a good reminder about : being dangerous here https://github.com/JuliaLang/julia/issues/35572

raphaelsaavedra avatar Aug 27 '20 14:08 raphaelsaavedra

I'm just going to throw out some alternate suggestions. First off I think some of the annoyances with breaking up exception messages over multiple lines is that it's easy to forget the space between lines. Having a string macro may be a nice way to alleviate that:

julia> macro fold_str(string)
           replace(string, '\n' => ' ')
       end
@fold_str (macro with 1 method)

julia> fold"""
               Whoa there. Things aren't all good.
               Doing the thing may lead to some seriously weird stuff happening.
               """
"Whoa there. Things aren't all good.   Doing the thing may lead to some seriously weird stuff happening. "

Also, maybe the what we should do for long exceptions is to have them defined as global constants. That could work well with folded multi-line strings. For messages that use variables from the surrounding scope we could possibly use a function instead which would probably help with performance as we'd have a function barrier in place.

With that all said I'm not opposed to having an exemption but it will be hard to define clear rules for this.

omus avatar Aug 27 '20 14:08 omus

I'll mention that there is now a MultilineStrings.jl package that makes it easier to write a single-line string with the multi-line syntax:

julia> m"""
       Whoa there. Things aren't all good.
       Doing the thing may lead to some seriously weird stuff happening.
       """
"Whoa there. Things aren't all good. Doing the thing may lead to some seriously weird stuff happening."

I think the way I would write the example in the description would now be:

function do_thing(status)
    is_good(status) && warn(LOGGER) do
        m"""
        Whoa there. Things aren't all good.
        Doing the thing may lead to some seriously weird stuff happening.
        """
    end
    return the_thing()
end

omus avatar Sep 25 '20 14:09 omus

See https://github.com/invenia/LibPQ.jl/pull/197/files#r494597781

A list of pairs where the second element of each pair is a long string of text is another good example of something that this would make nicer.

iamed2 avatar Sep 29 '20 22:09 iamed2

the string macro would not help with the fact that it just uses so much vertical space. which to me is the most important part of the problem

oxinabox avatar Sep 30 '20 11:09 oxinabox

The counter argument to this proposal is that having an exception for literal strings makes it harder to read error messages. Adding an exemption also means that people won't care as much about refactoring their logging messages to be terse and clear.

If the concern is about the logging messages taking up too much vertical space then using code folding in code editors is a simple way to address that concern. For very long strings developers should be considering making such a strings a constant or if interpolation is needed a function.

I will note I'm mostly playing devil's advocate but there are some dangers in adding this exemption. At this point I'm unconvinced the pros outweigh the cons.

omus avatar Sep 30 '20 13:09 omus

To be "devil's advocate" in the other direction: The 92 char line limit only makes sense if you also tell people what to do when you hit against that limit...

e.g. we say "if a ternary conditional x = a ? b : c doesn't fit within the line limit, instead use a mutli-line if-block"

and so on with advice for method definitions, arrays etc.

But we have nothing to say about string literals right now, so they get to break the line limit

nickrobinson251 avatar Sep 30 '20 14:09 nickrobinson251

But we have nothing to say about string literals right now, so they get to break the line limit

As always these are just guidelines and not rules. Some suggestions that I just made above could be more formally written as:

If a string literal exceeds the 92 character line limit try to break the string up over multiple lines. There are a few ways to do this including using the @m_str macro from MultilineStrings.jl. Alternatively, consider storing the string literal as a global constant or in a separate function if interpolation is required.

omus avatar Sep 30 '20 15:09 omus

If the concern is about the logging messages taking up too much vertical space then using code folding in code editors is a simple way to address that concern.

I am yet to find a code folding editor that works for julia well.

but there are some dangers in adding this exemption. At this point I'm unconvinced the pros outweigh the cons.

I also am at this point unconvinced that the pros out weigh the cons.

oxinabox avatar Sep 30 '20 15:09 oxinabox

my view is that string literals are special

i'm not sure whether or not they're special enough to warrant the exception

nickrobinson251 avatar Sep 30 '20 16:09 nickrobinson251