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

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

Open
jdesrosiers opened this issue Aug 14, 2024 · 9 comments
Open

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

jdesrosiers opened this issue Aug 14, 2024 · 9 comments
Labels
📝 Documentation Improvements or additions to documentation Status: Available No one has claimed responsibility for resolving this issue.

Comments

@jdesrosiers
Copy link
Member

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
Copy link
Collaborator

erosb commented Aug 14, 2024

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
Copy link
Member Author

jdesrosiers commented Aug 14, 2024

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
Copy link
Collaborator

erosb commented Aug 15, 2024

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
Copy link
Collaborator

erosb commented Aug 15, 2024

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
Copy link
Member Author

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 😉) that if/then is not implication, which not correct.

@JeelRajodiya JeelRajodiya added the 📝 Documentation Improvements or additions to documentation label Aug 25, 2024
@JeelRajodiya
Copy link
Member

JeelRajodiya commented Sep 6, 2024

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

JeelRajodiya commented Sep 7, 2024

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 (#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.

JeelRajodiya added a commit that referenced this issue Sep 9, 2024
@JeelRajodiya JeelRajodiya added the Status: Available No one has claimed responsibility for resolving this issue. label Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 Documentation Improvements or additions to documentation Status: Available No one has claimed responsibility for resolving this issue.
Projects
None yet
Development

No branches or pull requests

3 participants