Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

add rough docs for unevaluatedItems #205

Merged
merged 9 commits into from
Jul 5, 2023
Merged

add rough docs for unevaluatedItems #205

merged 9 commits into from
Jul 5, 2023

Conversation

micshar92
Copy link
Contributor

One thing that guided me was my concurrence with @jdesrosiers on #198:

I feel like UJS shouldn't be a cookbook of useful patterns and I feel like that's mostly what this is contributing. Since we don't have a cookbook, I'd be ok with merging this, but I do worry that it could be a slippery slope that results in ever increasing bloat

I included five examples: the ones I deemed to be the most common use cases. These were based on my conversations with others, a lot of Slack messages, and the unevaluateditems.json in the test suite repo. I linked to that document at the bottom of the page in a note box and gave a short list of other cases readers might need. Five examples was already adding bloat to the page, and I don't think I can part with any of them. What do you think?

Feel free to remove, edit, or add things. I validated and checked multiple times, but I could have made some grievous factual error. This is just a rough draft, and I'm still lacking in "professional experience." I hope the links and other style bits turn out right.

Oh, and you don't mind if I add a link on my portfolio site to prove I wrote this, do you?

@micshar92
Copy link
Contributor Author

@Julian @jdesrosiers Hello again. If you don't mind, please review my changes to the array.rst page. The new website (looking spiffy btw) is probably a priority at this moment, but I landed an interview for a good job recently and I feel my open-source experience maybe helped contribute to that. (of course it's just an interview) So I want to follow up here, even if the UJS website is soon to be overhauled. Thanks!

@Julian
Copy link
Member

Julian commented May 24, 2023

Sorry I missed this somehow. Will have a look!

@Julian
Copy link
Member

Julian commented May 24, 2023

Oh, and you don't mind if I add a link on my portfolio site to prove I wrote this, do you?

And definitely feel free to do that!

Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left you some notes, lemme know what you think!

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
@jdesrosiers
Copy link
Member

I'm looking forward to reviewing this when I'm back from vacation. Sorry for the delay.

source/reference/array.rst Outdated Show resolved Hide resolved
second draft of ``unevaluatedItems`` tutorial
@micshar92
Copy link
Contributor Author

micshar92 commented May 26, 2023

@Julian I don't know if people are automatically alerted when I submit a PR so I'm writing this comment.

Here's draft 2 of 3. Anything else to edit before the final? (@jdesrosiers I'll wait for you, no big deal)

I noticed, for instance, this section breaks style with the rest of the page because it has no red x or green check boxes. But I don't think that'll matter much.

@Julian
Copy link
Member

Julian commented May 26, 2023

Cool! I'll have another read.

I don't know if people are automatically alerted when I submit a PR so I'm writing this comment.

In general folks who are subscribed to the thread might get emails when you reply to individual comments and/or push commits, though definitely leaving a comment is still helpful/common, as people often are waiting for whenever you're done addressing a round of comments before doing another review (I certainly was) so yeah definitely will take another look now thanks again!

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
third draft of ``unevaluatedItems`` tutorial
@micshar92
Copy link
Contributor Author

@Julian @jdesrosiers Third and next-to-final draft up. See any improvements, let me know.

@Julian
Copy link
Member

Julian commented May 28, 2023

Nice! Took another skim, think this looks more or less good to me! Let's see what Jason thinks when he's back, I think next week.

Thanks again for the PR!

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for taking on this task. It's not an easy one.

For this first round of review, I'm just including syntax and code style issues. Make sure to build the site and preview locally to make sure everything is rendering as expected. See the README for instructions and let me know if you have any problems. I fixed an issue with the build this morning, so you'll want to rebase your branch before attempting the build.

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
fourth draft of ``unevaluatedItems`` tutorial
@micshar92
Copy link
Contributor Author

Fourth draft up.

See the README for instructions and let me know if you have any problems.

Well...
I just learned what Docker was yesterday, so my knowledge of it is lacking. Nonetheless, I got this error when trying to build in the VS Code terminal: (see attached image) So I can't build the site. Good to at least get experience with Docker, since I imagine it's a necessary technical writer skill. But do you know what this error means? To my knowledge, I followed the directions exactly.

error in building

@jdesrosiers
Copy link
Member

@micshar92 It looks like the build doesn't work in windows. Everything is harder in windows. I'm working on it. I have a hack that I think should get it working for you until I figure out what the real problem is.

In Dockerfile change, ENTRYPOINT ["./entrypoint.sh"] to CMD ["/bin/bash", "./entrypoint.sh"]. Then you'll need to use docker-compose up --build instead of docker-compose up every time you need to rebuild. Let me know if that doesn't work.

@micshar92
Copy link
Contributor Author

micshar92 commented May 31, 2023

Grrrr sorry I'm so incompetent! After about eight hours of trying to resolve why my localhost refused to connect, I restarted and fixed the problem somehow, but now after connecting, my browsers throw a 404. Surely I'm doing something stupid, right? (my backend app developer brother insists it's easier than I'm making it.)

Screenshot 2023-05-31 131216

@jdesrosiers
Copy link
Member

jdesrosiers commented Jun 3, 2023

@micshar92, don't feel bad, this was definitely not trivial to debug (and it's not on you to debug it anyway). The problem appears to be windows changing the line ending encoding. I think if you undo the hack I gave earlier and convert the line endings in entrypoint.sh from CRLF to LF, the build should work.

It looks like you might also need a fix I put in after you forked your branch, so you'll want to rebase your branch.

(In case you aren't familiar with rebasing ...)

git remote add upstream [email protected]:json-schema-org/understanding-json-schema
git pull --rebase upstream main
git push --force

@jdesrosiers
Copy link
Member

@micshar92 I merged the build fix to main. If you rebase (as described in the previous comment), hopefully your branch will build properly now.

@micshar92
Copy link
Contributor Author

It works... I'm just writing this because any moment now, I'll figure out how to get the changes to show up. It's still "documentation coming soon." Must be a button somewhere...

@jdesrosiers
Copy link
Member

@micshar92 It will build what is in your working directory. There's no delay or button to push. If you aren't seeing your changes, you might be on the wrong branch. Maybe you're still on my branch that you were using to help me test the build fix and haven't switched back to your branch where your changes are.

@micshar92
Copy link
Contributor Author

micshar92 commented Jun 5, 2023

Welp, thanks for being patient. Better I make mistakes like this before I secure employment.

Seems it was throwing errors. I fixed two lines of invalid JSON-- one with an extra comma, one missing a comma-- and did docker-compose build. But then I got to this one:

Screenshot 2023-06-05 183845

It referenced this:

{
  "oneOf": [
    { "prefixItems": [true] },
    { "unevaluatedItems": false }
  ]
}
--X
[1]

...but I don't see why this schema validates. Maybe you do.

Currently, I wrote it as "For instance, in the example below, the unevaluatedItems doesn't "see inside" the prefixItems cousin before it. So since "prefixItems": [ true ] matches only length 1 arrays, and { "unevaluatedItems": false } matches only empty arrays, all instances fail validation."

@jdesrosiers
Copy link
Member

For instance, in the example below, the unevaluatedItems doesn't "see inside" the prefixItems cousin before it. So since "prefixItems": [ true ] matches only length 1 arrays, and { "unevaluatedItems": false } matches only empty arrays, all instances fail validation.

[1] matches "prefixItems": [true] and doesn't match "unevaluatedItems": false. One of the schemas matches and the other doesn't, so oneOf passes. Perhaps you meant to use allOf rather than oneOf?

@micshar92
Copy link
Contributor Author

D'oh that fix was obvious. Anyway, now it runs perfectly. I fixed the note block at the bottom since they don't allow lists, it seems.

Screenshot 2023-06-06 153817

So everything is now complete and accurate, to my knowledge. I'll submit the pull request.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this has three main parts and the first two parts have a lot of issues. I think it would make more sense to start with something more like the third part. I would make start with a simple open tuple schema, then show various ways of referencing it ($ref) in other schemas to either extend it (as in part 1) or close it (as in part2) and maybe or maybe not both (as in part 3).

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
@micshar92
Copy link
Contributor Author

Things came up so this draft took awhile. Anyway, I'm running into this error now, and it doesn't make sense to me.

shouldnt be invalid

I ran that schema through my validator and it says there are no errors. I can't find any errors either. Am I missing something?

@Julian
Copy link
Member

Julian commented Jun 12, 2023

You have a trailing comma on the line with "$ref": "#" (which isn't valid JSON).

@micshar92
Copy link
Contributor Author

Aw man, these things are so obvious in hindsight. I've had this same problem in JavaScript but with semicolons. I'm glad Python doesn't have the semicolon at the end of the line.

@micshar92
Copy link
Contributor Author

Sixth draft up. I'm... not confident in it. It took me a couple hours, then a lightbulb went off-- surely unevaluatedItems is not as simple as I think it is now? Still, it looks great on the site. Just let me know if you have any improvements.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is better.

surely unevaluatedItems is not as simple as I think it is now?

It is that simple! In fact, there appear to be some bits that you still have a slightly over complicated mental model for. Go even simpler 😃

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
Seventh draft. Much simplified.

After I removed the reference to the test suite at the end, I toyed with adding some conclusion sentence-- it seems abrupt-- but it was just fluff, so I didn't. Anyway, here it is.
@micshar92
Copy link
Contributor Author

micshar92 commented Jun 14, 2023

Seventh draft. Much simplified. Any changes to make?

@micshar92
Copy link
Contributor Author

@Julian @jdesrosiers Busy? This could be the final draft before it's on the website.

source/reference/array.rst Outdated Show resolved Hide resolved
@jdesrosiers
Copy link
Member

Sorry for the delay. I have one more high-priority thing I have to get done before I can get to this. I expect to get to be able to review tomorrow.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're almost there. Thanks for sticking with it. This is a hard one.

source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
source/reference/array.rst Outdated Show resolved Hide resolved
@micshar92
Copy link
Contributor Author

Weird, I didn't get any notification of your responses. (due to the GitHub redesign?) I had to click over to the PR to see anything, so I just did these changes today.

Anyway, eighth (and maybe final?) draft is here. Everything's looking great.

Copy link
Member

@jdesrosiers jdesrosiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one small whitespace issue to fix, then I'll approve.

source/reference/array.rst Outdated Show resolved Hide resolved
Co-authored-by: Jason Desrosiers <[email protected]>
@micshar92
Copy link
Contributor Author

Dunno how that happened, but it's fixed now.

@micshar92
Copy link
Contributor Author

@Julian This appears to be done. Please approve.

Copy link
Member

@Julian Julian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did already but it still looks good to me! Thanks for the efforts.

@jdesrosiers jdesrosiers merged commit cf4f773 into json-schema-org:main Jul 5, 2023
@micshar92
Copy link
Contributor Author

Thank you. This was an adventure in learning.

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

Successfully merging this pull request may close these issues.

3 participants