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

Feature: Option to include data path in CLI output #46

Open
Lakitna opened this issue Aug 14, 2019 · 10 comments · May be fixed by #65
Open

Feature: Option to include data path in CLI output #46

Lakitna opened this issue Aug 14, 2019 · 10 comments · May be fixed by #65
Assignees
Projects
Milestone

Comments

@Lakitna
Copy link

Lakitna commented Aug 14, 2019

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 :)

@torifat torifat added this to the v1.0 milestone Sep 9, 2019
@torifat torifat added this to To do in v1.0 via automation Sep 9, 2019
@torifat torifat self-assigned this Sep 9, 2019
@torifat torifat moved this from To do to In progress in v1.0 Sep 10, 2019
@torifat
Copy link
Collaborator

torifat commented Sep 11, 2019

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

@torifat torifat moved this from In progress to To do in v1.0 Sep 15, 2019
@Lakitna
Copy link
Author

Lakitna commented Sep 21, 2019

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

@torifat
Copy link
Collaborator

torifat commented Sep 21, 2019

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

getCodeFrame(message, dataPath = this.options.dataPath) {

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.

@Lakitna
Copy link
Author

Lakitna commented Oct 13, 2019

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.

@torifat
Copy link
Collaborator

torifat commented Oct 14, 2019

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

@Lakitna
Copy link
Author

Lakitna commented Oct 15, 2019

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.

@torifat
Copy link
Collaborator

torifat commented Oct 16, 2019

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

@Lakitna Lakitna linked a pull request Oct 16, 2019 that will close this issue
3 tasks
@Lakitna
Copy link
Author

Lakitna commented Oct 16, 2019

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?

@torifat
Copy link
Collaborator

torifat commented Oct 17, 2019

Can you take care of that for me?

Sure, I can do that :)

@Lakitna
Copy link
Author

Lakitna commented Nov 18, 2019

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

orgads pushed a commit to orgads/better-ajv-errors that referenced this issue Oct 18, 2021
@torifat torifat added this to To do in v1.1 via automation Nov 8, 2021
@torifat torifat removed this from To do in v1.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v1.1
  
To do
Status: To do
Development

Successfully merging a pull request may close this issue.

2 participants