[PUP-8127] Refactor error location strings to not use words for better I18n support Created: 2017/11/07  Updated: 2018/02/05  Resolved: 2018/01/08

Status: Closed
Project: Puppet
Component/s: None
Affects Version/s: PUP 5.3.3
Fix Version/s: PUP 5.3.4, PUP 5.4.0

Type: Bug Priority: Normal
Reporter: Eric Delaney Assignee: Eric Delaney
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to INTL-42 I18n Review DiagnosticFormatter and D... Open
relates to PUP-8286 i18n review and add decoration to cas... Closed
Template: PUP Bug Template
Epic Link: Agent i18n/l10n testing
Sub-team: Coremunity
Team: Platform Core
Sprint: Platform Core KANBAN
Method Found: Inspection
Release Notes: Bug Fix
Release Notes Summary: The reporting of the location that an error was detect from is being updated in Puppet to enable better translations. The location is being change from using "at ", "at line", and "at file" to a statement of location '(file: <file>, line: <line>, column: <column>)'

For example:
Error: Something went wrong (file: /path//foo.pp:, line: 1, column: 9)
Error: Something went wrong (file: /path//foo.pp, line: 1)
Error: Something went wrong (line: 1, column :9)
Error: Something went wrong (line: 1)
QA Risk Assessment: No Action

 Description   

For better translations we should look at how we add the location information to an error in 'ExternalFileError'. Instead of using words like 'at', 'at line', and 'in' we should change to something like '(file:line:pos)'. Then we have no words to translate while still providing the location information.

I think replacing the output with the below. Based on which variables are set the names on the left, would cause the output on the right:

file line pos -> (file:line:pos)
file line -> (file:line)
line pos -> (line:pos)
line -> (line)

An example with all location information set

Error: Something went wrong (/Users/eric.delaney/work/my_repos/puppet/foo.pp:1:9)
Error: Something went wrong (/Users/eric.delaney/work/my_repos/puppet/foo.pp:1)
Error: Something went wrong (1:9)
Error: Something went wrong (1)

Places that need to be updated:
./puppet/error.rb - ExternalFileError -> to_s
./puppet/parser/resource.rb - def merge(resource)
./puppet/pops/validation.rb - DiagnosticFormatterPuppetStyle
./puppet/util/errors.rb - Puppet::Util::Errors -> def error_context
./puppet/util/log.rb - Puppet::Util::Log -> to_s line 403

Some of the code that needs to be updated:

  module ExternalFileError
    # This module implements logging with a filename and line number. Use this
    # for errors that need to report a location in a non-ruby file that we
    # parse.
    attr_accessor :line, :file, :pos
 
    # May be called with 3 arguments for message, file, line, and exception, or
    # 4 args including the position on the line.
    #
    def initialize(message, file=nil, line=nil, pos=nil, original=nil)
      if pos.kind_of? Exception
        original = pos
        pos = nil
      end
      super(message, original)
      @file = file unless (file.is_a?(String) && file.empty?)
      @line = line
      @pos = pos
    end
    def to_s
      msg = super
      @file = nil if (@file.is_a?(String) && @file.empty?)
      if @file and @line and @pos
        "#{msg} at #{@file}:#{@line}:#{@pos}"
      elsif @file and @line
        "#{msg} at #{@file}:#{@line}"
      elsif @line and @pos
          "#{msg} at line #{@line}:#{@pos}"
      elsif @line
        "#{msg} at line #{@line}"
      elsif @file
        "#{msg} in #{@file}"
      else
        msg
      end
    end
  end



 Comments   
Comment by Henrik Lindberg [ 2017/11/08 ]

I think there are additional places where almost the same thing is taking place. In general the use of various error types is a mess, not only for i18n purposes. It is a big thing to refactor all of it.

Comment by Eric Delaney [ 2017/11/08 ]

Henrik Lindberg Ok, I was thinking we could find the places that we call it with the options file, line, or pos and convert them to pass a complete message instead and not pass the extra arguments. Essentially move the to_s() logic into the caller, and the just not pass in the options. At some future point you could remove the extra error logic.

Would the be helpful or more painful later?

Comment by Henrik Lindberg [ 2017/11/08 ]

The message and the file/line does not necessarily come from the same place - a typical case is that a lower level error occurs and the logic that raises it has no clue as to where it is in terms of "file and line in puppet" when that occurs. Instead the error is caught and the file/line information is added.

Then, the error is both logged and formatted into the error message you see. The logging may be structured in which case file and line are not made into part of the message - that information goes into other slots in the format.

Thus, we cannot simply put file/line into the message at the point where the error is raised.

My preference would be that we changed the output to not include any words at all for file/line, just punctuation say (path line:pos), or (line:pos) or similar. If we must have words we do need to produce the "at file, line:pos" separately.

Comment by Eric Delaney [ 2017/11/17 ]

Henrik Lindberg I see what you mean. So I've updated the ticket what do you think of the suggested change in the description now?

Comment by Henrik Lindberg [ 2017/11/17 ]

I am fine with the proposed change - it is however problematic to do as it breaks "api" and potentially screws up a lot of tests that rely on the exact output from error messages. I also think we also need to consider those that parse the output of error messages to programatically "understand" the output - for that reason we may want to format the output to make it possible to have filenames with spaces in them, or indeed a filename that happens to end with something like (1:5. In the past that was difficult, and we should at least not make it worse.

Some of the combination shown in the description are not possible - for instance a pos without a line does never occur in practice (this because pos is "position on line", and if that is known, it is also known which line it is on).

Comment by Eric Delaney [ 2017/11/17 ]

Henrik LindbergOk so if we wanted to make it parsable by a script then we need to always how about using ':' between entries and always including them.

Error: This expression is invalid. Did you try declaring a 'crazy' resource without a title? (/Users/eric.delaney/work/my_repos/puppet/foo.pp:1:9)
Error: This expression is invalid. Did you try declaring a 'crazy' resource without a title? (:1:9)
Error: This expression is invalid. Did you try declaring a 'crazy' resource without a title? (:1

From a readability standpoint I'm not sure I like that, other than we already have that now with

"#{msg} at #{@file}:#{@line}:#{@pos}"

we just don't have the "at" any more.

Comment by Henrik Lindberg [ 2017/11/17 ]

How about something like this? (Brackets are more distinctive than parentheses IMO), and ':' is only used when there is something to delimit, and pos is never on its own. (I shortened the message in the examples to make them not be truncated in the ticket. Also did not end with '.' or '?' (many errors do not have ending punctuation.)

Error: Something went wrong [/Users/eric.delaney/work/my_repos/puppet/foo.pp:1:9]
Error: Something went wrong [/Users/eric.delaney/work/my_repos/puppet/foo.pp:1]
Error: Something went wrong [1:9]
Error: Something went wrong [1]
Error: Something went wrong [<from-commandline>:1:9]

The messages without file are rare and then there is usually no line or pos either, because error is not related to one particular file. Missing information is most often the result when running tests, or when the puppet code comes from the command line or Puppet[:code] setting. We could fix those to use a symbolic name instead of a filename (this is done in some places so the file is shown as <unknown> or similar). No overall approach has been taken to this, I think this could be improved upon but requires some thought and digging into the various forms of errors.

Comment by Eric Delaney [ 2017/11/17 ]

Josh Cooper Do you have an opinion on this?

Comment by Henrik Lindberg [ 2017/11/18 ]

How do we deal with errors with references to files and positions elsewhere in the puppet eko system? Should really do the same everywhere for consistency/UX sake.

Comment by Eric Delaney [ 2017/11/20 ]

Java/clojure uses ()'s

Exception in thread "main" java.lang.RuntimeException: Too many arguments to if, compiling:(bob.clj:6:5)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6875)
	at clojure.lang.Compiler.analyze(Compiler.java:6669)
	at clojure.lang.Compiler.analyze(Compiler.java:6625)

clang:

test.c:28:8: warning: extra tokens at end of #endif directive [-Wextra-tokens]

ruby:

t.rb:2: syntax error, unexpected keyword_end, expecting end-of-input

python:

eric.delaney ~ $ python t.rb
  File "t.rb", line 2
    ["a"].each end
                 ^
SyntaxError: invalid syntax

Comment by Eric Delaney [ 2018/01/04 ]

Merged to 5.3.x at https://github.com/puppetlabs/puppet/commit/165a7997029582016c4a4277bec4829ae0d58626

Generated at Thu Feb 27 00:45:15 PST 2020 using JIRA 7.7.1#77002-sha1:e75ca93d5574d9409c0630b81c894d9065296414.