json-schema-org / tour

A tour of JSON Schema
https://tour.json-schema.org
Other
23 stars 18 forks source link

The implication lesson doesn't use the implication pattern #63

Open jdesrosiers opened 3 months ago

jdesrosiers commented 3 months ago

The implication lesson uses if/then as an example instead of implication.

{
  "type": "object",
  "properties": {...},
  "if": {"properties": {"isFullTime": {"const": true}}},
  "then": {"required": ["salary"]},
}

Implication uses the pattern,

{
  "anyOf": [
    { "not": { ... if schema ... } },
    { ... then schema ... }
  ]
}

I suggest removing this lesson from the the tour. The tour states that it's for 2020-12. The if/then keywords were introduced to be a better way to express a conditional than implication. Since 2020-12 has if/then, there's no reason to use implication any more. In any case, there's no way to test that the user actually uses implication rather than if/then because they evaluate the same.

erosb commented 3 months ago

Hello @jdesrosiers

Since 2020-12 has if/then, there's no reason to use implication any more.

I'm not sure if I understand this. if/then is implication, isn't it?

jdesrosiers commented 3 months ago

I see what you mean. Technically it is, but it's confusing because we've always referred to "the Implication Pattern" (!A or B) as the alternative to if/then when you don't have access to those keywords. I see now what you meant, and it makes sense, but it seems unnecessary to bring in the concept of implication (that few people recognize) when talking about if/then (that's familiar to everyone). Can you think of a reason why describing it as implication is beneficial?

erosb commented 3 months ago

If my memories from first-order formal logic are correct, implication is if A then B. Actually, the not A or B is the result of implication elimination , so it isn't implication itself (reference Converting from first-order logic section, step 1.1).

I understand that calling not A or B as implication was part of an internal jargon in the past, before the "if"/"then"/"else" keywords existed, but I'm not convinced we need to stick to that in learning material designed for new-joiners.

We can rename the chapter if you insist to if-then-else , would you be OK with that?

erosb commented 3 months ago

Regarding this:

In any case, there's no way to test that the user actually uses implication rather than if/then because they evaluate the same.

@JeelRajodiya can you please add a validation schema here, which verifies that the user-supplied schema does contain the "if" and "then" keywords? AFAIR we introduced validation schemas to address these kind of problems.

jdesrosiers commented 3 months ago

Actually, the not A or B is the result of implication elimination , so it isn't implication itself

I can't agree with that take. They are two different expressions of the same concept. I've always understood the implication operator (->) to be a shorthand because implication is such a common and useful pattern in boolean algebra (reference). The concept isn't bound to the operator. The operator is just a convenience. "Implication elimination" refers to eliminating the implication operator, which can be useful in cases where it's not convenient.

We can rename the chapter if you insist to if-then-else , would you be OK with that?

I'm not insisting on anything, just trying to help. As I said, I understand now what you were doing and it makes sense. I was just confused because we've never in this community referred to if/then as implication (although you're right that it is). If you decide to keep it the way it is, we should consider updating the documentation that presents implication as an alternative to if/then. Maybe we should update the documentation in any case since it implies (pun intended :wink:) that if/then is not implication, which not correct.

JeelRajodiya commented 2 months ago

Regarding this:

In any case, there's no way to test that the user actually uses implication rather than if/then because they evaluate the same.

@JeelRajodiya can you please add a validation schema here, which verifies that the user-supplied schema does contain the "if" and "then" keywords? AFAIR we introduced validation schemas to address these kind of problems.

We have removed the support for validation schema, since in some cases it added extra complexity to the project and gave confusing error messages. We can figure out an other way if we decide to keep this lesson

JeelRajodiya commented 2 months ago

@jdesrosiers considering the content of the tour should be easy to understand for the beginners. I have taken used if/then for implication from the this section of the docs (see below):

image https://json-schema.org/understanding-json-schema/reference/conditionals#implication

This pattern seemed more easy to understand compared to the original implication pattern suggested in the docs.

If I understand you correctly. we already have lesson for if/then/else. so a dedicated lesson for if/then makes seems redundant. Am I correct? I think we should keep this lesson because it shows the users that they can use if/then/else without the else, what do you think?

jdesrosiers commented 2 months ago

If I understand you correctly. we already have lesson for if/then/else. so a dedicated lesson for if/then makes seems redundant. Am I correct?

No, that's not what I meant. I thought the intention of this lesson was to teach the if/then alternative that that section of the documentation you linked to describes. I didn't realize that using if/then was the point of the lesson, so I didn't even notice that there were two lessons teaching almost the same thing.

But now that you bring it up, I don't think there needs to be two lessons, but I would keep this one, and drop the other. I think this lesson is important to teach that the required keyword is necessary (#66). That's a very common mistake. If you ask me, that's the most important concept that needs to be covered. Instead of a whole other lesson for else, I'd just include a note that else also exists and works how you'd expect already knowing how then works. In my experience, else is very rare anyway. If you decide to keep both lessons, I would put if/then before if/then/else. At least that way the lessons build on each other (starting with if/then and then adding else in the next lesson).

JeelRajodiya commented 2 months ago

I suggest removing this lesson from the the tour. The tour states that it's for 2020-12. The if/then keywords were introduced to be a better way to express a conditional than implication. Since 2020-12 has if/then, there's no reason to use implication any more. In any case, there's no way to test that the user actually uses implication rather than if/then because they evaluate the same.

We can rename the chapter to if-then instead of implication and mention that if-then can also be used as implication (similar to the official docs).

I think this lesson is important to teach that the required keyword is necessary (https://github.com/json-schema-org/tour/issues/66).

Yes, I will make necessary changes for it.

If you decide to keep both lessons, I would put if/then before if/then/else. At least that way the lessons build on each other (starting with if/then and then adding else in the next lesson).

Sounds good to me. We should rearrange it. if We do not see much interest from the users (through analytics) we can think of removing the if-then-else lesson.