How should we handle long exception messages ? #218

greg0ire · 2016-12-13T21:01:07Z

⚠️ Disclaimer: this is going to be very pedantic and totally qualifies as bike-shedding.

That being said… I had an interesting discussion with @sagikazarmark this week, about exception messages.

Given a long exception message, long being > 120 characters or whatever makes you run out of breath, how should it be handled in the code?

Let's take a real-world example, slightly altered to add a quote ('), which we'll talk about later.
I can imagine three ways to handle this:

The original way : no splitting at all

<?php
throw new InvalidArgumentException(sprintf('Unable to choose a translation for "%s" with locale "%s" for value "%d". Double check that this translation has the correct plural options (e.g. "There\'s one apple|There are %%count%% apples").', $message, $locale, $number));

Splitting of the input string only with concatenation

<?php
throw new InvalidArgumentException(sprintf(
  'Unable to choose a translation for "%s" with locale "%s" for value "%d".'.
  ' Double check that this translation has the correct plural options'.
  ' (e.g. "There\'s one apple|There are %%count%% apples").',
  $message,
  $locale,
  $number
));

Splitting of both strings with nowdoc

<?php
throw new InvalidArgumentException(sprintf(
  <<<'EOT'
Unable to choose a translation for "%s" with locale "%s" for value "%d".
Double check that this translation has the correct plural options
(e.g. "There's one apple|There are %%count%% apples").
EOT
  ,
  $message,
  $locale,
  $number));

Solutions 2 and 3 are easier to read IMO, since you don't have to use horizontal scrolling to do so, but also because humans naturally read short lines more easily (don't just believe me, follow the link). If you've configured your IDE to wrap the text, it's still easier to read because the string wasn't split at random, but around logical connectors, following the natural rhythm of the sentence.

But the following question arises : by who is this string meant to be read? Primarily by the users of the library, that's true. Does it mean that developers should consider it as pure data, to be ignored by other developers of the library?

To that question, I answer two things:

There is also code review that matters, and if you use github, it happens without any word wrapping. And error messages should be thoroughly reviewed, shouldn't they? So don't make it hard.
Granted, error messages are less important that the code that surrounds them, but they matter nonetheless to the developer, because they help him understand the surrounding code, a bit like comments do. So we should treat them like comments, because they are not just data, they carry relevant meaning in them. If you think otherwise, then maybe we should find a way to move them outside the code, or at least isolate them somewhere.

Another reason to prefer solution 1 would be "greppability" (don't look it up, I made that one up): If you have to take carriage returns into account when searching for the code that generates an error, things are going to get impractical. But then again, if you split the string at logical connectors, this problem is unlikely to arise. Searching for ". Double check or options (e.g. " is probably not what people will be doing.

My personal preference goes to solution 3, because I think it makes the string itself easier to parse for a human : no need to worry about concatenation, preserving spaces between sentences, or escaping. The string will look very similar to the output string (more than in version 1, where you have to escape quotes or double quotes, your choice). Also, it makes the text easier to edit: you can navigate it more quickly, and add several words without having to maintain the concatenation, all you have to do is pick carriage returns. And if you do that, the diff will be easier to read, be it at code review or when some random developer reads the history.

A final argument I can think of maybe is that it's good to have one error message = one log line, but nowadays, with Sentry / ELK, logs are no longer consider as lines and more as JSON objects.

Thoughts? Insults? Calls for my internment in some asylum ?

The text was updated successfully, but these errors were encountered:

sagikazarmark · 2016-12-14T02:47:15Z

I started a poll about it:
https://twitter.com/sagikazarmark/status/808757703250219008

OskarStark · 2017-01-10T10:58:32Z

My personal preference goes to solution 3

mine, too 👍

greg0ire · 2017-01-10T11:00:25Z

Thank you @OskarStark I was starting to feel very lonely! Now I know I'm not completely crazy!

goetas · 2017-01-11T17:27:23Z

My personal preference goes to solution 2 or 1 (in order of preference)

3 is the last option i will consider...

greg0ire · 2017-01-11T17:29:53Z

Ok, but why?

goetas · 2017-01-11T17:38:28Z

partially is just personal preference... do not have a strict position on it.

<<<'EOT' and similar syntaxes are valid, but for me are always a bit "weird" and have to jump to the documentation to see if the syntax is the one i have in mind....
and in personal experience, many developers never used them in years of work.

One side effect of the syntax is that breaks indentation... as example:

if (condition){
   throw new InvalidArgumentException(sprintf(
<<<'EOT'
Unable to choose a translation for "%s" with locale "%s" for value "%d".
Double check that this translation has the correct plural options
(e.g. "There's one apple|There are %%count%% apples").'
EOT
  ,
  $message,
  $locale,
  $number));
}

//is different from 

if (condition){
    throw new InvalidArgumentException(sprintf(
<<<'EOT'
   Unable to choose a translation for "%s" with locale "%s" for value "%d".
   Double check that this translation has the correct plural options
   (e.g. "There's one apple|There are %%count%% apples").'
EOT
   ,
   $message,
   $locale,
   $number));
}

as i remember, te string will contain "tabs or spaces" in the message depending on the indentation used (and in both cases will contain newlines.. and if you do not want newlines, will have to use solution 1 or 2 proposed by you

goetas · 2017-01-11T17:43:49Z

Even in your example:

<<<'EOT'
Unable to choose a translation for "%s" with locale "%s" for value "%d".
Double check that this translation has the correct plural options
(e.g. "There's one apple|There are %%count%% apples").' <<<<<----this
EOT

the last quote should not be there...

greg0ire · 2017-01-11T17:45:46Z

partially is just personal preference... do not have a strict position on it.

<<<'EOT' and similar syntaxes are valid, but for me are always a bit "weird" and have to jump to the documentation to see if the syntax is the one i have in mind....
and in personal experience, many developers never used them in years of work.

I completely agree with you, this is seldom used, but maybe we need to use it more? This kind of syntax exists in every language (well at least I encountered in bash, python and puppet). Not using it because it is never used is kind of a vicious circle, isn't it?

as i remember, te string will contain "tabs or spaces" in the message depending on the indentation used (and in both cases will contain newlines.. and if you do not want newlines, will have to use solution 1 or 2 proposed by you

That's a good point, sadly heredoc is not as powerful in php as it is in some other languages where this the tabs or space problem is solved. But then again if we never use it there is little incentive to improve it.

greg0ire · 2017-01-11T17:46:26Z

the last quote should not be there...

Thanks, fixed it (copy/paste mistake)

goetas · 2017-01-11T17:51:43Z

I completely agree with you, this is seldom used, but maybe we need to use it more? This kind of syntax exists in every language (well at least I encountered in bash, python and puppet). Not using it because it is never used is kind of a vicious circle, isn't it?

using a feature (just) because is available do not look the right argument to me...

Unfortunately the problem with tabs, newlines and whitespaces is pretty important to me, so i see it as a "blocker" unless a solution is found

As said my personal preference goes to proposal 2, that solves the issue with a commonly known syntax (just string concatenation)

goetas · 2017-01-11T17:53:21Z

But kudos for thinking about the problem! I faced the same issue many many times, and always solved it "somehow"... Now i'm more aware of problem and solution :)

greg0ire · 2017-01-11T17:54:54Z

using a feature (just) because is available do not look the right argument to me...

Yeah it's not really an argument indeed, I feel that for many people, it's just indeed a habit problem and that just getting them to use heredoc in a few spots of their code would radically change how they view it. I mean look at this : https://github.com/sonata-project/SonataAdminBundle/blob/b776d7d27ae5f46959ce41caae1563fd3aaac7e1/Tests/Twig/Extension/SonataAdminExtensionTest.php#L660 It was written without heredoc before, with all the quotes escaped (or was it the double quotes ?)

Unfortunately the problem with tabs, newlines and not necessary namespaces is pretty important to me, so i see it as a "blocker" unless a solution is found

Well that I can understand, way more rational that any argument I read so far.

goetas · 2017-01-11T18:01:42Z

I mean look at this : https://github.com/sonata-project/SonataAdminBundle/blob/b776d7d27ae5f46959ce41caae1563fd3aaac7e1/Tests/Twig/Extension/SonataAdminExtensionTest.php#L660 It was written without heredoc before, with all the quotes escaped (or was it the double quotes ?)

this is the perfect case for heredocs! personaly in this cased i also indent the code, anyway in html spaces are ignored (almost always)

greg0ire · 2017-01-11T18:40:43Z

The heredoc end marker would break the flow anyway so I fully went with it since there is less risk to get lines that are beyond 120 chars (the limit github uses for its editor). But it would indeed be great if we had more things like this. These guys got it just right.

goetas · 2017-01-11T18:46:05Z

hehe nice :) but fortunately php has not some "bad" things that puppet has ;)

greg0ire · 2017-01-11T18:47:20Z

Yeah puppet is quite weird otherwise 😅 but this particular thing… nice!

goetas · 2017-01-11T18:54:59Z

do not hate me for this meme 😅

greg0ire · 2017-01-11T18:56:37Z

do not hate me for this meme 😅

I don't ;) I am using puppet for legacy things, am using new things to ansible, and am very close to using docker in production, so…

sagikazarmark · 2017-01-11T21:53:27Z

@greg0ire you ask for reasoning, but keep in mind that in the end this is mostly personal taste and preference.

greg0ire · 2017-01-11T22:49:53Z

Well now that I got some reasoning based on facts, I can see how people can come to dislike heredoc / nowdoc. I'd stick to it for my projects because my personal preference is to value the advantages I exposed over not breaking the indention / not having carriage returns in the output string, however, in Sonata, I'd be willing to settle for solution 2 since it looks like people are more confortable with it. It's hard to write, but at least, it's easy to read / review. But solution 1, really… it's off limits. All it does is remind me of http://stopwritingramblingcommitmessages.com/

soullivaneuh · 2017-01-12T13:43:22Z

Insults?

🔫

More seriously: You will hate me, but we should following Symfony coding style AFAIK.

So exception messages should be on an unique line.

soullivaneuh · 2017-01-12T13:44:30Z

And BTW, I quite agree with that. This is easier to find the corresponding message on the code with Ctrl+F with on-lined messages.

greg0ire · 2017-01-12T13:50:07Z

lol did GH use a water pistol for 🔫 ?

More seriously: You will hate me, but we should following Symfony coding style AFAIK.

Hate the sin, not the sinner. And you know my views on that… if Symfony makes a bad decision they can't change easily, I don't think it's a good idea to be consistent with that decision. Don't let the disease spread.

And BTW, I quite agree with that. This is easier to find the corresponding message on the code with Ctrl+F with on-lined messages.

That's a good Pro for solution 1. It does not have many, but at least that one is good (I listed it in my original post, in case you didn't notice).

greg0ire · 2017-01-30T16:20:20Z

I think we should opt for solution 2, it's a bit like democracy : it does not make everyone happy, but it's the least harmful system there seems to be.

Also, @core23 asks (see sonata-project/SonataBlockBundle#359) when we should linebreak sentences. I recall discussing this with @soullivaneuh a while ago, and we agreed semantic wrapping would be best.

sagikazarmark · 2017-01-31T22:13:46Z

Is this discussion still a thing?

I would just cut it short with a project owner vote -> done. This problem doesn't really matter for anyone, but the ones who develop the project.

greg0ire · 2017-01-31T23:29:08Z

You're right, let's get it over with! @sonata-project/contributors , please upvote the solution(s) that are okay with you.

greg0ire · 2017-01-31T23:29:30Z

Solution 1

greg0ire · 2017-01-31T23:29:35Z

Solution 2

greg0ire · 2017-01-31T23:29:39Z

Solution 3

soullivaneuh · 2017-02-03T09:23:47Z

I would just cut it short with a project owner vote -> done.

You mean @rande?

sagikazarmark · 2017-02-05T10:34:08Z

Whoever is supposed to make these decisions: if you are a democratic team, then a team vote, otherwise it's the dictator's choice.

greg0ire · 2017-02-05T14:31:11Z

We're a democratic team guys, right? Right?

greg0ire · 2017-02-20T20:57:18Z

To solve this, I'd advise everyone to vote for solution 2, which makes no one super happy, but no one super unhappy either, and is pretty much what we are doing right now anyway. And please remove your downvotes, it complicates everything. Please do something, this is agonizing…

greg0ire · 2017-03-07T09:48:30Z

@jordisala1991 can you cast your vote here?

jordisala1991 · 2017-03-07T09:53:04Z

Done.

greg0ire · 2017-03-07T10:15:27Z

Ok so now all solutions have equal votes (if we ignore the downvote, which we should). Can everyone agree on solution 2? Please upvote or downvote this message.

sagikazarmark · 2017-03-07T11:12:40Z

@greg0ire I think this vote is biased enough by your guidance and "recommendations" already. Note that solution two still has the most downvotes. I don't really care anymore if it's option 1, 2 or 3 though.

greg0ire · 2017-03-07T11:17:12Z

@sagikazarmark anyone is free to add guidance and recommendations and should express themselves. I'm the more vocal about this, but I think it's better if people know what I think before voting. Maybe they didn't think of everything? Anyway, with 3 votes out of 4 members interested in this enough to vote previously, I think this is settled. Solution 2 it is.

greg0ire · 2017-11-17T22:33:01Z

Looks like php is starting to get heredoc right too: https://wiki.php.net/rfc/flexible_heredoc_nowdoc_syntaxes

greg0ire mentioned this issue Dec 13, 2016

Add missing deprecations sonata-project/SonataFormatterBundle#222

Merged

greg0ire mentioned this issue Dec 15, 2016

remove SecurityContextInterface it was removed in symfony 3.0 sonata-project/SonataUserBundle#812

Merged

greg0ire mentioned this issue Jan 10, 2017

Feature: CmsPageRouter can now render pages from other sites by chang… sonata-project/SonataPageBundle#748

Merged

core23 mentioned this issue Jan 30, 2017

Avoid long lines sonata-project/SonataBlockBundle#359

Merged

greg0ire mentioned this issue Feb 20, 2017

Removed SonataClassificationBundle in a BC way sonata-project/SonataMediaBundle#1201

Merged

1 task

greg0ire closed this as completed Mar 7, 2017

greg0ire mentioned this issue Sep 26, 2017

Support unlimited nested child admins. sonata-project/SonataAdminBundle#4542

Merged

4 tasks

greg0ire mentioned this issue Oct 23, 2017

Show deprecation when using unsupported twig getAttribute function sonata-project/SonataCacheBundle#162

Merged

How should we handle long exception messages ? #218

How should we handle long exception messages ? #218

Comments

greg0ire commented Dec 13, 2016 • edited Loading

sagikazarmark commented Dec 14, 2016 • edited Loading

OskarStark commented Jan 10, 2017

greg0ire commented Jan 10, 2017

goetas commented Jan 11, 2017 • edited Loading

greg0ire commented Jan 11, 2017

goetas commented Jan 11, 2017 • edited Loading

goetas commented Jan 11, 2017

greg0ire commented Jan 11, 2017 • edited Loading

greg0ire commented Jan 11, 2017

goetas commented Jan 11, 2017 • edited Loading

goetas commented Jan 11, 2017

greg0ire commented Jan 11, 2017

goetas commented Jan 11, 2017

greg0ire commented Jan 11, 2017

goetas commented Jan 11, 2017

greg0ire commented Jan 11, 2017

goetas commented Jan 11, 2017

greg0ire commented Jan 11, 2017 • edited Loading

sagikazarmark commented Jan 11, 2017

greg0ire commented Jan 11, 2017 • edited Loading

soullivaneuh commented Jan 12, 2017

soullivaneuh commented Jan 12, 2017

greg0ire commented Jan 12, 2017 • edited Loading

greg0ire commented Jan 30, 2017 • edited Loading

sagikazarmark commented Jan 31, 2017

greg0ire commented Jan 31, 2017 • edited Loading

greg0ire commented Jan 31, 2017

greg0ire commented Jan 31, 2017

greg0ire commented Jan 31, 2017

soullivaneuh commented Feb 3, 2017

sagikazarmark commented Feb 5, 2017

greg0ire commented Feb 5, 2017

greg0ire commented Feb 20, 2017 • edited Loading

greg0ire commented Mar 7, 2017

jordisala1991 commented Mar 7, 2017

greg0ire commented Mar 7, 2017

sagikazarmark commented Mar 7, 2017

greg0ire commented Mar 7, 2017

greg0ire commented Nov 17, 2017

greg0ire commented Dec 13, 2016 •

edited

Loading

sagikazarmark commented Dec 14, 2016 •

edited

Loading

goetas commented Jan 11, 2017 •

edited

Loading

goetas commented Jan 11, 2017 •

edited

Loading

greg0ire commented Jan 11, 2017 •

edited

Loading

goetas commented Jan 11, 2017 •

edited

Loading

greg0ire commented Jan 11, 2017 •

edited

Loading

greg0ire commented Jan 11, 2017 •

edited

Loading

greg0ire commented Jan 12, 2017 •

edited

Loading

greg0ire commented Jan 30, 2017 •

edited

Loading

greg0ire commented Jan 31, 2017 •

edited

Loading

greg0ire commented Feb 20, 2017 •

edited

Loading