better-ajv-errors icon indicating copy to clipboard operation
better-ajv-errors copied to clipboard

Feature: Option to include data path in CLI output

Open Lakitna opened this issue 5 years ago • 10 comments

Data paths are great when working with big, repeating or complex responses, but at the moment these paths are not included in the CLI output. It would also be very useful if you don't have the indent option set.

I'd like to have an option to enable data path output in the CLI mode. It might look something like.

const output = betterAjvErrors(schema, response.body, ajv.errors, {
    indent: 2,
    dataPath: true,
});
console.log(output);
     Error: FORMAT should match format "date-time"

   5 |         "foo": "624050509",
   6 |         "bar": "04",
>  7 |         "baz": "not-a-date-time",
     |                ^^^^^^^^^^^^^^^^^ 👈🏽  format should match format "date-time"
   8 |         "lorum": "@",
   9 |         "ipsum": "some string",
  10 |         "razz": 100797.51,

     @ /a/long/path/9/with/2/big/21/arrays/baz

And sure, the example above is a really extreme example, and if your API looks like this you have bigger problems. But these things exist in the wild, so I'd like to at least be able to test them with some ease. This feature would contribute to this.

I'm willing to implement this myself if someone can tell me where to start :)

Lakitna avatar Aug 14 '19 15:08 Lakitna

Hi @Lakitna, do you still want to work on this? And, how can I help you? :)

torifat avatar Sep 11 '19 10:09 torifat

@torifat If you can point me to where I should probably start I can probably figure things out from there.

Lakitna avatar Sep 21 '19 08:09 Lakitna

You can refactor getCodeFrame to use this.options.dataPath and print the path with the code frame.

https://github.com/atlassian/better-ajv-errors/blob/126ee2e45ee829e69e3b2f3f6d56eb49d5c42135/src/validation-errors/base.js#L34

Just a heads up, I'm converting the whole project to TypeScript: https://github.com/torifat/better-ajv-errors/tree/typescript. So, you welcome to make this change in either place.

torifat avatar Sep 21 '19 12:09 torifat

I looked at this today and got it working (see screenshot below). But right now without the option since there does not seem to be an existing way to pass options to that part of the code. Should I create a way to pass the option or just make this the default behaviour without a way to disable it? I prefer to make this default as it is useful information.

image

Once we've made a decision I'll implement it and work on the tests.

Lakitna avatar Oct 13 '19 21:10 Lakitna

I was planning to make it a default behavior for v1 :)

torifat avatar Oct 14 '19 04:10 torifat

Some tests are failing and it seems to be unrelated to my changes. Can you make sense of errors like the one below? These tests already failed before I changed anything. It's probably an OS thing. I'm running Windows 10 and these tests don't fail on CI.

image

Probably be a good idea to fix this if I can.

Edit: Traced it back to json-to-ast, probably not fixable.

Lakitna avatar Oct 15 '19 17:10 Lakitna

If doesn't fail in CI then feel free to create the PR. I will follow up on the json-to-ast thing. Also, I was planning to replace it anyways. See #51

torifat avatar Oct 16 '19 02:10 torifat

In that case, everything is done but the documentation.

I've tried to update the screenshots, but that does not seem to work on Windows. Can you take care of that for me?

Lakitna avatar Oct 16 '19 14:10 Lakitna

Can you take care of that for me?

Sure, I can do that :)

torifat avatar Oct 17 '19 06:10 torifat

Just a heads up that this is ready for merge after the screenshot update as far as I'm concerned.

Lakitna avatar Nov 18 '19 10:11 Lakitna