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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Configuration schema is not valid" - missing feedback why it is invalid #1526

Closed
donaldpipowitch opened this Issue Nov 23, 2018 · 14 comments

Comments

Projects
None yet
4 participants
@donaldpipowitch
Copy link

donaldpipowitch commented Nov 23, 2018

Propose a new feature/change

Write a short description of your proposal with (if applicable)
some examples of the expected behaviour.

Hi there 馃憢

I just try out hint for the first time. My first use case is running WCAG2A checks. Props for the nice documentation about that 馃憤

I haven't used hint before and so I stripped out the invalid JSON parts and got a configuration like this one:

{
  "hints": {
    "axe": ["error", {
      "runOnly": {
        "type": "tag",
        "values": ["wcag2a"]
      }
    }]
  }
}

But running $ yarn hint http://localhost:9001/ shows this:

Configuration schema is not valid
Couldn't find any valid configuration

I guess it is, because connectors and formatters are mandatory? (Because they can be seen here, but without concrete values. However in the configuration documentation it is not highlighted what is actually mandatory.) It would be nice, if the feedback could include the following parts:

  1. What was validated? E.g. change "Configuration schema is not valid" to something like "Configuration schema of /path-to/.hintc is not valid", so I know my configuration was actually validated. (I think it was picked up correctly, but I'm not sure.)
  2. Why is it invalid? E.g. change "Configuration schema is not valid" to something like "Configuration schema is not valid. The following fields are missing: connectors, formatters." (Similar to the way how Webpack gives feedback to invalid configurations.)

@donaldpipowitch donaldpipowitch changed the title "Configuration schema is not valid" misses feedback _why_ it is invalid "Configuration schema is not valid" - missing feedback why it is invalid Nov 23, 2018

@molant

This comment has been minimized.

Copy link
Member

molant commented Nov 23, 2018

@sarvaje sarvaje self-assigned this Dec 21, 2018

@antross antross added this to the 1812-2 milestone Dec 21, 2018

sarvaje added a commit to sarvaje/hint that referenced this issue Dec 22, 2018

sarvaje added a commit to sarvaje/hint that referenced this issue Dec 22, 2018

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 4, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 4, 2019

I'm working on this, and there is some cases where the result can be a little bit confusing. The results are "prettified" from the result that ajv return.
For example, if we change the value of a hint to an invalid numeric value e.g. 8, this is the result:
image

The last error, I think it is related to the other 'path' in the schema that is not failing, I mean, hints property can be a hintobject or an array, but because what we have in the config is an invalid hintobject, then the validator return than the property hints should be array.

The same logic is apply to the error number 2.

In this case, IMO, the ideal result would be, or just the first message, or all the messages joined with or, but in this case, the message will be huge.

@webhintio/core thoughts?

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 4, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 5, 2019

@sarvaje sarvaje referenced this issue Jan 5, 2019

Merged

Fix: Error returned when the config is invalid. #1602

5 of 6 tasks complete
@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 5, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 5, 2019

For me the Or is not weird, because the property can be one thing "or" another.

The problem is that "hints" can be a lot of things hehehe.

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 5, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 5, 2019

Anyways, the message for sure that may need some changes, but that is not the "issue" I was trying to expose the thing is if message 2 and 3 are necessary, or if message 2 should go with message 1, etc.

@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 5, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 7, 2019

and... how do you know what message is the one that fits better the user intention? I mean, ajv return an array of errors, and maybe we can ignore the message number 3, because the datapath is a sub path of the datapath in message 1 and 2, but how do you know that 1 fit better the user intention than 2?

@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 7, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 7, 2019

I don't know, I need to make more tests.

@molant molant modified the milestones: 1812-2, 1901-1 Jan 7, 2019

@antross

This comment has been minimized.

Copy link
Member

antross commented Jan 7, 2019

Even if the order seems consistent at the moment, depending on the order of the messages is likely a fragile approach so I'd recommend we don't take it. Getting some feedback to the user here, even if the string isn't exactly what we want, is better than none.

To improve the messages in the future, it may be worth us filing an issue with ajv to see if either we're missing something obvious or if there's an improvement we can help make to get better messages.

@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 8, 2019

Even if the order seems consistent at the moment, depending on the order of the messages is likely a fragile approach

What I wanted to know is if ajv will output first the error that's most likely to be based on the type of the property. With @sarvaje's example, if the value is 8 get the errors related to the numbers first, if it's a string, then error, warning, off, etc. If that's the case we can probably use the first message. If that's not the case and the order is always the same no matter the used type, then we should just output whatever we receive.

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 8, 2019

The order of the errors depends on the schema file. For example, if we modify the file packages/hint/src/lib/config/config-schema.json editing the property 'hint->anyOf' to:

"anyOf": [
                {
                    "type": "array",
                    "items": {
                        "anyOf": [
                            {
                                "type": "string"
                            },
                            {
                                "type": "array",
                                "items": [
                                    {
                                        "type": "string"
                                    },
                                    {
                                        "type": "object"
                                    }
                                ],
                                "minItems": 2,
                                "maxItems": 2
                            }
                        ]
                    },
                    "minItems": 1
                },
                {
                    "$ref": "#/definitions/hintobject"
                }
            ],

The error number 3 would be the number 1.

@molant

This comment has been minimized.

Copy link
Member

molant commented Jan 8, 2019

@sarvaje

This comment has been minimized.

Copy link
Member

sarvaje commented Jan 8, 2019

Me too.

In that case, I think #1602 is ready to review. :)

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 11, 2019

sarvaje added a commit to sarvaje/hint that referenced this issue Jan 12, 2019

@molant molant closed this in #1602 Jan 14, 2019

molant added a commit that referenced this issue Jan 14, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment