Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exempting literal strings from line limits? #59

Open
nickrobinson251 opened this issue Aug 27, 2020 · 13 comments
Open

Exempting literal strings from line limits? #59

nickrobinson251 opened this issue Aug 27, 2020 · 13 comments

Comments

@nickrobinson251
Copy link
Contributor

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
@iamed2
Copy link
Collaborator

iamed2 commented Aug 27, 2020

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?

@nickrobinson251
Copy link
Contributor Author

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?

@iamed2
Copy link
Collaborator

iamed2 commented Aug 27, 2020

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

@raphaelsaavedra
Copy link
Contributor

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 JuliaLang/julia#35572

@omus
Copy link
Contributor

omus commented Aug 27, 2020

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
Copy link
Contributor

omus commented Sep 25, 2020

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

@iamed2
Copy link
Collaborator

iamed2 commented Sep 29, 2020

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.

@oxinabox
Copy link
Member

oxinabox commented Sep 30, 2020

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

@omus
Copy link
Contributor

omus commented Sep 30, 2020

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.

@nickrobinson251
Copy link
Contributor Author

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

@omus
Copy link
Contributor

omus commented Sep 30, 2020

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.

@oxinabox
Copy link
Member

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.

@nickrobinson251
Copy link
Contributor Author

my view is that string literals are special

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants