Skip to content

Remove default.json because default is not intended for validation #141

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

Closed
wants to merge 1 commit into from

Conversation

pipobscure
Copy link

Since default is not used for validation, it should not be in the test-suite.

See #139 as well as json-schema-org/json-schema-spec#204

@Julian
Copy link
Member

Julian commented Dec 26, 2016

These tests are still correct though. Why would we want to remove them? They're testing that implementations do not misinterpret default, which yes is not used for validation.

@handrews
Copy link
Contributor

What @Julian said. Line 19 in the first file and other similar lines should perhaps be changed to not say "still valid when invalid default is used" but something like "still valid in the presence of an invalid default"

@Julian
Copy link
Member

Julian commented Dec 26, 2016

Yeah the wording seems not great.

@pipobscure
Copy link
Author

pipobscure commented Dec 26, 2016

The reason to remove them is simply that their presence suggests to implementors that default has a meaning regarding validation. Since it does not, these tests are misleading.

@Julian
Copy link
Member

Julian commented Dec 26, 2016

@pipobscure in what way are they misleading though? These tests, if an implementation does not pass, indicates the implementation is not spec-conforming. That's the essence of tests, to indicate whether an implementation is correct or not :)

@handrews
Copy link
Contributor

They are trying to catch the situation where someone assumes that a default must validate. Since that is in fact a common misconception, it's a valid test.

@pipobscure
Copy link
Author

@Julian they are null tests though. We might just as well at these tests then:

[
    {
        "description": "invalid type for julian",
        "schema": {
            "properties": {
                "foo": {
                    "type": "integer",
                    "julian": []
                }
            }
        },
        "tests": [
            {
                "description": "valid when property is specified",
                "data": {"foo": 13},
                "valid": true
            },
            {
                "description": "still valid when the invalid julian is used",
                "data": {},
                "valid": true
            }
        ]
    },
    {
        "description": "invalid string value for julian",
        "schema": {
            "properties": {
                "bar": {
                    "type": "string",
                    "minLength": 4,
                    "default": "bad"
                }
            }
        },
        "tests": [
            {
                "description": "valid when property is specified",
                "data": {"bar": "good"},
                "valid": true
            },
            {
                "description": "still valid when the invalid julian is used",
                "data": {},
                "valid": true
            }
        ]
    }
]

The julian keyword has no relevance at all, so why test for it.

On the other hand default is part of the specification, so the test has merit, but then we also need to add tests for description and title and all the other annotations.

@Julian
Copy link
Member

Julian commented Dec 26, 2016

@pipobscure they are not null tests :)

It seems like you mean to say "it is obvious that this behavior is correct because default is only for documentation", but we started exactly on the opposite side of that :), with the tests pointing out that fact itself.

The goal of a test is to ensure that an implementor is properly informed about incorrect assumptions they may have made. It is the closest to an explicit specification as you can possibly get reasonably.

If in fact anyone was worried that some implementor was going to introduce improper behavior for a non-existent julian property, yes precisely those tests should exist :), but there is no such worry, so they do not.

default however is a continuing source of confusion both for implementors and users, and the tests that are here (and perhaps even some that are missing) are exactly to make sure that no one makes a mistake in a place where such mistakes are rampant.

@pipobscure
Copy link
Author

@handrews @Julian : OK, then. Let's just agree that the state of definition of default is a total mess. It's there, but has no value to validation whatever other than that it's not necessary to be valid. (Which also holds true of anything else I put into a schema).

I'd be free to add a standard property to a schema. That could/could-not need validate just like default. In other words, in the current state of the spec default is entirely superfluous and does nothing more than confuse implementors.

So I guess the best way forward would be to remove the default keyword from the spec altogether.

@pipobscure pipobscure closed this Dec 26, 2016
@handrews
Copy link
Contributor

@pipobscure it's not superfluous (although it is very unclearly written). It reserves the keyword for a specific sort of use, but does not constrain that use. It prevents someone else from imposing other behavior on it, and it prevents different implementations from picking different keywords for the same sort of thing. The SHOULD is also of note- it tells users that doing something other than what SHOULD advises can result in strange and useless behavior.

@pipobscure
Copy link
Author

@handrews well it forces anyone that requires a valid default to not rely on default but create something custom. So in other words, we now have to create a non-standard keyword standard which MUST be valid, because we can't rely on default to be valid.

In other words, what this achieves is exactly the opposite of what is intended.

@Julian
Copy link
Member

Julian commented Dec 26, 2016

That is precisely what was intended. It forces anyone who wants to do something other than what is defined to use something else, and standardizes what is defined for anyone who does not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants