raml-org / raml-tck

Test Compatibility Kit for RAML 1.0
http://raml-org.github.io/raml-tck/
8 stars 10 forks source link

Errors trace #56

Closed KonstantinSviridov closed 8 years ago

KonstantinSviridov commented 8 years ago

In some cases assotiating an error with particular position in code is not correct or inconvenient.

For example:

#%RAML 1.0
title: New API
types:
  TestType:
    type: object
    properties:
      prop1: number
    example: !include examples/example.raml
#examples/example.raml
prop1: stringValue

It's not correct to place error only in example.raml line 1, because it's validity depends on type using it. Placing a single error on the example: !include examples/example.raml line is not handy for the user, as he might expect more details.

In such cases Workbench provides error trace: trace2

The suggestion is to introduce the trace into TCK JSON. For the example above errors would be

"errors": [
    {
      "code": 11,
      "message": "number is expected",
      "path": "api.raml",
      "line": 7,
      "column": 4,
      "position": 102,
      "range": [
        {
          "line": 7,
          "column": 4,
          "position": 102
        },
        {
          "line": 7,
          "column": 11,
          "position": 109
        }
      ],
      "trace": [
        {
          "code": 11,
          "message": "number is expected",
          "path": "examples/example.raml",
          "line": 0,
          "column": 0,
          "position": 0,
          "range": [
            {
              "line": 0,
              "column": 0,
              "position": 0
            },
            {
              "line": 0,
              "column": 18,
              "position": 18
            }
          ]
        }
      ]
    }
  ]
KonstantinSviridov commented 8 years ago

@sichvoge , @svacas , @evangelina , what do you think?

svacas commented 8 years ago

I like the trace idea, but I think the error should be reported where it actually occurred (in this case example.raml) and the trace would contain all the include chain till the root raml

sichvoge commented 8 years ago

What @svacas said plus that we might need some different colouring for traces since it might be confusing that it currently not differently handled compared to errors. What I mean is that the line example: !include examples/example.raml is actually not really wrong and it might not make sense to mark it in this way.

sichvoge commented 8 years ago

BTW, is this really TCK related or more the parser output?

KonstantinSviridov commented 8 years ago

@sichvoge The only thing related to TCK is errors format in TCK JSON. For errors highlighting we may have a separate ticket in the workbench repository.

KonstantinSviridov commented 8 years ago

Reversing errors order is not difficult, but it will cause the need to update tools which already use the JSON. For example, the API Designer expects an error in the root file to be on top level. I guess, we should ask @juancoen for his opinion.

KonstantinSviridov commented 8 years ago

One more example. Here it is clear, which error is primary.

#api.raml
#%RAML 1.0

title: test
resourceTypes:
  rt: !include resourceTypes/rt.raml

# resourceTypes/rt.raml
post:
  body:
    application/json:
      properties:
        p1: string
      example:
        p1: 5

Now the error does not have any trave, and it is

{
      "code": 11,
      "message": "string is expected",
      "path": "api.raml",
      "line": 4,
      "column": 2,
      "position": 45,
      "range": [
        {
          "line": 4,
          "column": 2,
          "position": 45
        },
        {
          "line": 4,
          "column": 4,
          "position": 47
        }
      ]
    }

With the reversed trace it is going to be

{
      "code": 11,
      "message": "string is expected",
      "path": "resourceTypes/rt.raml",
      "line": 6,
      "column": 8,
      "position": 102,
      "range": [
        {
          "line": 6,
          "column": 8,
          "position": 102
        },
        {
          "line": 6,
          "column": 10,
          "position": 104
        }
      ],
      "trace": [
        {
          "code": 11,
          "message": "string is expected",
          "path": "api.raml",
          "line": 4,
          "column": 2,
          "position": 45,
          "range": [
            {
              "line": 4,
              "column": 2,
              "position": 45
            },
            {
              "line": 4,
              "column": 4,
              "position": 47
            }
          ]
        }
      ]
    }
sichvoge commented 8 years ago

Can we move that issue to the js-parser please? Let's not mix up the TCK with the parser output even if that output originally came from the TCK.

@KonstantinSviridov

KonstantinSviridov commented 8 years ago

@sichvoge The purpose of creating the issue here is to coordinate JSON format. As the JSON format has been accepted by each side, the issue can indeed be moved to JS-parser.

sichvoge commented 8 years ago

The JSON format is the output of the parser and should therefore be discussed there, not the TCK. It just happened that the format came out of the TCK discussion, but now the TCK maps to the parser output format. Hence, all discussions around the parser output should be in the parser.

sichvoge commented 8 years ago

Moved that out into this issue and closing this one.