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

In error messages, print the source fragment responsible for the error #2096

Merged
merged 9 commits into from Oct 21, 2018

Conversation

Projects
None yet
@Armael
Copy link
Member

commented Oct 8, 2018

This PR generalizes to the standard compiler output the "source highlighting" mechanism that is already present in case of error in a "dumb" toplevel and in the testsuite expect tests.

Examples:
single_line
multi_line2

The motivation is to improve error readability: it should now be clearer what the mistake is when looking at the compiler output. AFAIK elm, rust or bucklescript implement similar error rendering, and were inspiration for this work.


The PR reuses the existing code for quoting source code, making a few improvements:

  • Line numbers in a left column
  • Color when underlining a single-line location, reusing the color for error/warning
    warning
  • Ellipsis in case of multi-line locations with too many lines (10 is the limit)
    multi_line_ellipsis

The code for the highlighting function got a bit more complicated, because we now have to be able to read the source to quote from the input file. The current function reads directly from the lexbuf and only works if it hasn't been refilled in the meantime -- which is reasonable for the toplevel but not so much for a .ml file. There are also a lot of changes in the testsuite, that come in a separate commit.

On the plus side, one consequence of these changes is the merge of three different codepaths for error printing into one, which makes me very happy. Specifically, "batch mode", "dumb toplevel" and "expect test" now use exactly the same code for printing errors.

(string * int) list
=
(* Converts a global position to one that is relative to the lexing buffer *)
let rel n = n - lb.lex_abs_pos in

This comment has been minimized.

Copy link
@Armael

Armael Oct 8, 2018

Author Member

Hum, I suspect this is in general incorrect, because I'm not taking lb.lex_start_pos into account?

This comment has been minimized.

Copy link
@gasche

This comment has been minimized.

Copy link
@diml

diml Oct 9, 2018

Member

If I read the code of lexing.ml correctly, I believe it should indeed be something like: n - lb.lex_abs_pos + lb.lex_start_pos. Does the current code work?

BTW, I find looking directly into the lexing buffer internals a bit tedious. Personally, I would just keep the whole input as a single string and extract the source code from that string, that would make the code simpler.

This comment has been minimized.

Copy link
@Armael

Armael Oct 9, 2018

Author Member

If I read the code of lexing.ml correctly, I believe it should indeed be something like: n - lb.lex_abs_pos + lb.lex_start_pos.

Yeah, that's what I suspected after reading lexing.ml a bit more closely

Does the current code work?

..yes :) [on the testsuite]

BTW, I find looking directly into the lexing buffer internals a bit tedious.

Agreed.

I would just keep the whole input as a single string and extract the source code from that string

Hum, which part of the code would take care of that? (note that the lexbuf business is only really needed for the toplevel, otherwise we can just go read the source file)

This comment has been minimized.

Copy link
@diml

diml Oct 9, 2018

Member

The code that processes an input file. But yh, I agree that for plain files it's not an issue. For the toplevel, the code would need to be adapted. Without changing everything, we could keep all the input read since the start of the current loop iteration in a Buffer.t.

This comment has been minimized.

Copy link
@Armael

Armael Oct 9, 2018

Author Member

@diml I just tried your suggestion, and it breaks the function (either stops quoting the source, or quotes wrong parts of it) :D. So I guess the current implementation is correct?

This comment has been minimized.

Copy link
@diml

diml Oct 10, 2018

Member

I guess so. Thanks for trying, at least we know the current one is more likely to be right :)

@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

This is superb! I've wanted this the whole time I've used OCaml!

At the risk of asking to gild the lily, might I suggest also moving towards a more standard error location format as well? The FSF coding standards, which are followed by gcc and llvm and many other compilers, specify that line numbers and columns be printed as

sourcefile:lineno:column: message

or alternatively as:

sourcefile:lineno.column: message

See: https://www.gnu.org/prep/standards/html_node/Errors.html

Doing it that way makes it easier for tools to parse the mentioned locations.

That said, if this notion is controversial, please ignore it. I don't want to slow down getting error context printed for the user.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 8, 2018

I think this is orthogonal to this PR (it is not touching the File ..., Line ..., Characters ... line) so I would keep it for somewhere else :-).

@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 8, 2018

@Armael Fair enough. But it should be done at some point, and given that so many tests suite error messages are going to have to get changed anyway to deal with the changed output, it would be good to do it all at about the same time.

@gasche
Copy link
Member

left a comment

This looks very nice, thanks! Some comments from a first read.

let highlight_quote ppf
~(get_lines:
start_pos:position -> end_pos:position ->
(string (* line *) * int (* beginning of line *)) list)

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

Why not use a record type instead of comments in the type?

This comment has been minimized.

Copy link
@Armael

Armael Oct 9, 2018

Author Member

Ehh, why not, but that sounds a bit heavywheight.

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

If you feel the need to indicate what the values are for, I think that record or object fields are in order.

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Done.

let lines =
get_lines ~start_pos:leftmost ~end_pos:rightmost
|> List.map (fun (line, s) ->
line, try_get_line_number s (s + String.length line), s)

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

We don't expect this part of the compiler to be performance-critical, but I'm still a bit uneasy with the quadratic runtime of this code. Isn't there a linear or nlogn version? (It feels like you are only keeping a set around.)

| Some line_nb -> max acc (String.length (string_of_int line_nb))
| None -> acc
) 0 lines
in

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

If you could separate the display code to go in utils/Misc, that could maybe be reused more easily later? I think what you are really implementing is two-column printing with a separator, with a parametrized justification function. I guess that Arthur would have reused that.

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

(Besides this whole highlight_quote function breaks the "don't mix different abstraction levels" rule for good code. We shouldn't get from list manipulations of location sets to low-level padding code without a transition, we should have auxiliary functions for the logic of each domain.)

| [(line, line_nb_opt, line_start_cnum)] ->
(* Single-line error *)
Format.fprintf ppf "%a%s@," (prefix ~sep:true) line_nb_opt line;
Format.fprintf ppf "%a" (prefix ~sep:false) None;

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

What's the benefit of the ~sep:true ... ~sep:false protocol over just having a command in-between to print the separator?

This comment has been minimized.

Copy link
@Armael

Armael Oct 9, 2018

Author Member

Yeah, I got rid of that.

highlighted := false;
Format.fprintf ppf "@}"
end;
if is_start pos then begin

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

I'm again uneasy with the linear lookup here, giving quadratic behavior. We could have a set of start positions.

let ellipsed_first, ellipsed_last =
let lines_nb = List.length lines in
if lines_nb > max_lines then
let printed_lines = max_lines - 1 in (* ellipsis uses one line *)

This comment has been minimized.

Copy link
@gasche

gasche Oct 8, 2018

Member

the ellipsis

This comment has been minimized.

Copy link
@Armael

Armael Oct 9, 2018

Author Member

Fixed, thanks.

Show resolved Hide resolved parsing/location.ml
(string * int) list
=
(* Converts a global position to one that is relative to the lexing buffer *)
let rel n = n - lb.lex_abs_pos in

This comment has been minimized.

Copy link
@gasche
Show resolved Hide resolved parsing/location.ml
@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

Thanks for the quick first review @gasche . I agree that the code grew into an untasteful mess in some places, I'll work on cleaning that up today.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2018

@gasche I just pushed a commit that moves the code for printing two columns into Misc, and also tries to address the "quadratic behavior" issues.

I'm not completely satisfied with the second part, though. The new code works by defining a structure for a "bag of start/end positions", created from a list of locations, and by providing cursors that allow traversing this bag in O(l) where l is the number of locs that have been put in the bag.
This sounded nice in principle, but the code was a bit tricky to get right (cursors are tricky it seems!), and feels a bit overkill. Especially since what you call quadratic is in fact O(n*l) where n is the number of characters in the output, and l the number of locations -- and l is in practice 1 or 2 at the moment.

But even ignoring the complexity, I agree the previous code was ugly and had to be rewritten...
I'll let you judge and comment on the new one.

@Armael Armael force-pushed the Armael:quote_location2 branch from 5baed97 to 068216c Oct 9, 2018

List.iter (function
| `Start, _ -> Format.fprintf ppf "@{<%s>" highlight_tag
| `End, _ -> Format.fprintf ppf "@}"
) ps;

This comment has been minimized.

Copy link
@gasche

gasche Oct 10, 2018

Member

Does that really work correctly when there are multiple overlapping locations positions? I might, but it looks a bit subtle.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

I agree that the cursor approach is a bit weird. From the change to the Bag structure, I like the impact on the Location code (nice factorization), not so much the cursor API, and the implementation indeed looks complex. Also, I don't want to waste your time over-designing an efficient solution when there is no strong need for now.

So I would suggest a compromise:

  • pick a simpler, nicer API that can be efficiently implemented (and whose use gives nicely readable code)
  • do a naive implementation based on your knowledge that the set of locations will always remain very small

My suggestion would be : imagine that you have an efficient implementation of an interval tree. The natural API would be to ask for leftmost and rightmost interval, and also whether at least one interval in the bag overlaps with a given absolute position (and maybe to request the list of overlapping intervals). Then implementing this with lists as you suggest could be ok.

(Another approach would be to consider that we don't need to represent overlapping intervals in the structure, as they can always be fused at structure-construction time. Then a Map indexed on the starting position would be enough. But I don't remember if that is enough for your needs, and I'm not sure that trying to be both clever and specialized at the same time is a good idea.)

Show resolved Hide resolved driver/pparse.ml
@gasche

This comment has been minimized.

Copy link
Member

commented Oct 10, 2018

In principle I approve of the change; we need to converge on a version the code in location.ml that @Armael and myself feel good about.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

Take 3. (the first commit is from #2103, I'll rebase one it is merged)
I think the set-of-locations business is reasonable now.

There is one issue remaining: check-typo (rightfully) complains about the tests because some of them now use more than 80 columns. Indeed, if an error happens in a 80 col line of code, the error message will contain 42 | theline which is more than 80 characters long.
What should we do?

@gasche
Copy link
Member

left a comment

I still have some minor comments, but overall this looks good, thanks! The other PR has been merged so you can rebase now.

For line limits, check-typo distinguishes "long lines" (>80) and "very long lines" alarms (>130), so we could conceivably say that long lines that are not very long are ok, or add an "overflowing_line" category at 85. None of this is very hard, but correctly adding this alarm-marker to all testsuite files is tricky for now, before #1910 is merged (and I'm the one that has to turn the crank there).

Another approach would be to print lines that are two long differently:

 1 | foo bar
 2 |
very long foo bar
 3 | foo baz
val of_intervals : (('a * int) * ('a * int)) list -> 'a t

val mem : 'a t -> pos:int -> bool
val find_in : 'a t -> s:int -> e:int -> 'a option

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

At first I couldn't tell what s and e were for here. Maybe just int * int, or more explicit label names.

The specification, if I understand the implementation correctly, is: return the value associated to one of the interval bounds that lies within the (s, e) parameters. Maybe find_bound_in?

This comment has been minimized.

Copy link
@trefis

trefis Oct 17, 2018

Contributor

Agreed, s and e are just bad names.
More explicit label names are indeed required (and I'd be in favor adding a label even if the function is going to take a pair of ints).

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Should be better now (val find_bound_in : 'a t -> range:(int * int) -> 'a bound option).

Changes Outdated
@@ -121,6 +121,9 @@ Working version

### Compiler user-interface and warnings:

- PR#????: Add source highlighting for errors & warnings in batch mode
(Armaël Guéneau, review by ?)

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

That's Jérémie and me.

end
=
struct
type 'a boundary = 'a * int

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

In fact you could expose this type and have find_boundary_in return it.

overlap).
*)

module IMapSet : sig

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

Why IMapSet?

| `E, `Inside (s, n) -> `Inside (s, n-1), acc
) (`Outside, []) pos in
if nesting <> `Outside then
raise (Invalid_argument "ISet.of_intervals: invalid intervals");

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

When are intervals invalid in this sense?

Should the exception name be changed?

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

If I read the implementation correctly, I don't see how this exception can ever be raised given that the bounds only inserted when the S is before the E. (So we have never read more Es than Ss, and we have always read exactly at much at the end). So it could be an assert (nesting = Outside)`.

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Indeed, the exceptions were actually impossible cases. I turned them into assert false.

implementation if needed.
Note: the structure only stores maximal intervals (that therefore do not
overlap).

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

Out of curiosity, does the "maximal intervals only" aspect matter much? (It adds some complexity in of_intervals, and if I understand correctly it means that some of the user-provided bound annotations are lost.)

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Right. I think it simplifies the logic for inserting @{<..> and @} later, which will get trickier if it has to handled possibly overlapping intervals (and I think the previous implementation which tried to do it was incorrect).

So I think it's better to merge intervals directly in the interval set structure, additionally that's what an actual interval set implementation would do I believe.

let highlight_quote ppf
~(get_lines:
start_pos:position -> end_pos:position ->
(string (* line *) * int (* beginning of line *)) list)

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

If you feel the need to indicate what the values are for, I think that record or object fields are in order.

Format.fprintf ppf "%s | %s@," line_nb line;
Format.fprintf ppf "%*s " (String.length line_nb) "";
for pos = line_start_cnum to rightmost.pos_cnum - 1 do
(* NB: The two cases below are mutually exclusive *)

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

There are four cases below, which two do we mean? Maybe use else if if the tests are exclusive?

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Indeed, that wasn't clear. I used an else if as you suggest.

It first tries to read from [!input_lexbuf], then if that fails (because the
lexbuf no longers contains the input we want), it reads from [!input_name]
directly *)
let get_lines_from_current_input ~start_pos ~end_pos =

This comment has been minimized.

Copy link
@gasche

gasche Oct 17, 2018

Member

lines_around_from_lexbuf, lines_around_from_file, lines_around_from_current_input?

This comment has been minimized.

Copy link
@Armael

Armael Oct 18, 2018

Author Member

Done.

Show resolved Hide resolved parsing/location.ml

@gasche gasche referenced this pull request Oct 18, 2018

Merged

Simpler check-typo #1910

@Armael Armael force-pushed the Armael:quote_location2 branch from 2919111 to 570d84a Oct 18, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I'm getting fairly satisfied with the implementation, but here is an UI question: suppose a user strongly dislikes the new error formatting (maybe the extra lines break their workflow in some way), is there an easy way for them to disable it?

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

(If there is, maybe it could be mentioned in the Changes entry.)

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

I think I addressed all of your comments on the code!

I still have to look at the long lines issue. I'm not so fond of the idea of printing long lines differently, it does not look too good, and it only makes a difference for lines between 76 (or so) and 80 characters long...

It does not look too hard to set the long-line attribute on all testsuite files, maybe I could do that?
Or maybe, reformat the code of the tests so that the error message do not overflows anymore?

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

suppose a user strongly dislikes the new error formatting (maybe the extra lines break their workflow in some way), is there an easy way for them to disable it?

There isn't, currently. It's not hard to implement, but we have to think carefully if 1) we need it (adding alternative codepaths has a maintenance cost) 2) how to pass this option (as an environment variable? a commandline flag?)

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

It does not look too hard to set the long-line attribute on all testsuite files, maybe I could do that?

It seems reasonable to me, but I think it would be better to wait for #1910 to be merged (hopefully today or tomorrow) to do it.

There isn't [a way to disable the feature].

I think that an environment variable could be nice, but on the other hand the complexity of adding the configuration-plumbing makes me as uneasy as you. Maybe we could merge as-is and see what is the feedback in trunk testing and beta versions.

@Armael Armael force-pushed the Armael:quote_location2 branch from 570d84a to 80521ed Oct 18, 2018

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

I imagine that people who compile from inside an editor or IDE probably don't want this output, since the editor is going to highlight the code in your buffer anyway. So an option to disable it would probably be appreciated.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2018

Right, although note that anyone using merlin (directly or through LSP) should not be affected (merlin will disable this part of the message directly through the compiler-libs api).

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

My feeling is that Leo is speaking the voice of reason on this one.

You could start with just an OCAML_QUOTE_SOURCE environment variable, or if you want the full command-line-parameter dance just look at how -color and OCAML_COLOR are handled in the compiler.

@pmetzger

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

FYI, I don't think an option to disable such messages exists in other compilers I use, and almost all of them produce output like this now.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

While my first gut feeling was @lpw25's one I'm not entirely sure --- but would need to live with it to definitively answer the question. Here are a few points to ponder:

  1. Your editor can only highlight a single error/warning and you may not want to jump through all the locations to assess the nature of a bunch of errors.
  2. The direct vicinity between the error message and the source extract might actually help to figure out the problem more quickly before jumping to the actual location; your eyes don't have to repeatedly travel between two different areas (error panel/source panel).
Show resolved Hide resolved driver/compenv.ml Outdated
Show resolved Hide resolved driver/compmisc.ml

@Armael Armael force-pushed the Armael:quote_location2 branch from c36feaa to 392a791 Oct 20, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

Right. I exposed env_reader, and moved the reader definitions to Clflags. I also reused the usage message in compenv.ml as you pointed out, which is nice.

@Armael Armael force-pushed the Armael:quote_location2 branch from 392a791 to f2f3ab8 Oct 20, 2018

@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2018

And I just amended the docstring of the option to mention the existence of the environment variable.

Show resolved Hide resolved driver/main_args.ml Outdated
@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

It may make sense to have a third auto value supported as a command-line option (but either unsupported or ignored by the environment-value parser) whose semantics is just to cascade/inherit the environment value (or other future default-guesses we could make), and use contextual if absent.

@gasche Just to make things a bit clearer. The way usually these kind of cli API is implemented/expected (and supported by e.g. cmdliner) is as follows:

Your program has a setting which has a default value and whose value can be changed on the cli via optional(s) argument(s). If the setting has a corresponding environment variable, setting the environment variable only changes the default of the setting and that's all (i.e. a cli specification always "wins").

auto in the case of colors is something else. auto means that the tool is trying to be smart about choosing the final behaviour (in the case of colors by consulting the result of isatty(3) on stdout or stderr) but it is a value for the setting as any other value (and can also be set in the environment variable, though that is useless since auto is the default, unless I'm mistaken).

@gasche

gasche approved these changes Oct 20, 2018

Copy link
Member

left a comment

I made two minor code comments but in general I'm fairly happy with the shape of the code now. I'm marking this Approved to show progress, although of course we should wait for the error-style naming suggestions to converge before merging.

Show resolved Hide resolved parsing/location.ml Outdated
Show resolved Hide resolved toplevel/opttopmain.ml Outdated
@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2018

I would also like to add that in this particular example build systems should always only go for the default. This allows end-users (or opam) to force the format they actually desire.

@Armael Armael force-pushed the Armael:quote_location2 branch from f2f3ab8 to dd5b131 Oct 20, 2018

Show resolved Hide resolved driver/main_args.ml Outdated
Show resolved Hide resolved driver/main_args.ml Outdated

@Armael Armael force-pushed the Armael:quote_location2 branch from dd5b131 to 6b16bcc Oct 20, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 20, 2018

It is my understanding that all issues have been resolved. Unless I hear otherwise, I plan to merge if CI passes.

(Thanks @dbuenzli for the naming help!)

@gasche gasche merged commit f1e5730 into ocaml:trunk Oct 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Armael

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2018

Many thanks to everyone involved!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

Commit ef76e6e has just been pushed to
trunk to update two test reference files that had been forgotten.
They are used in -no-flat-float-array mode.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@Armael @shindere : This GPR appears to break the testsuite when OCAML_ERROR_STYLE is not set to the default. Could you please fix this for 4.08?

cc @damiendoligez

@Armael

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

What do you suggest?
By design changing OCAML_ERROR_STYLE changes the output of all error messages emitted by the compiler; so indeed it will break all the tests that emit error messages.
Do you have a specific fix in mind?

@Armael

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@trefis tells me on IRC that we should just make sure ocamltest explicitly sets the OCAML_ERROR_STYLE environment variable to the default.
This sounds fairly easy (and a job for @shindere :-)).

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@trefis

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

I'd like to point out that I didn't suggest that part:

This sounds fairly easy (and a job for @shindere :-)).

What I will suggest however is making sure that other variables which can affect the testsuite are handled the same way, e.g. OCAMLRUNPARAM.

@Armael

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@mshinwell I'll try to look at that tomorrow and implement @trefis suggestions (have ocamltest explicitly unset the environment variables that can modify the compiler output).

@shindere

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@Armael

This comment has been minimized.

Copy link
Member Author

commented Feb 27, 2019

See #2271 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.