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

Already on GitHub? Sign in to your account

Shorter, more focused referencing guide #80

Merged
merged 3 commits into from
Mar 12, 2024
Merged

Shorter, more focused referencing guide #80

merged 3 commits into from
Mar 12, 2024

Conversation

handrews
Copy link
Member

@handrews handrews commented Feb 5, 2024

This reworks the less-controversial parts of a prevoius attempt (#69) at a referencing guide that was over-ambitious, among other things. It's amazing how being in better health makes my writing less cranky and confrontational! 馃槄

This reworks the less-controversial parts of a prevoius attempt
at a referencing guide that was over-ambitious, among other things.
Copy link

@notEthan notEthan left a comment

Choose a reason for hiding this comment

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

馃憤

referencing/faq.md Outdated Show resolved Hide resolved
referencing/faq.md Outdated Show resolved Hide resolved
| 3.x | Path Item Object | `"$ref"` | allowed under some circumstances | logically replace the Path Item Object containing the `"$ref"` with a Path Item Object that combines the fields of the target Path Item Object with the non-`"$ref"` fields of the Path Item Object containing the `"$ref"`, as long as none of those fields conflict |
| 3.1 | Schema Object | `"$ref"` | allowed | Apply the target Schema Object to the same instance location as the Schema Object containing the `"$ref"`, and combine the results with the results of other keywords in the Schema Object containing the `"$ref"` just as you would any other keyword results; this is more-or-less equivalent to using a one-element `"allOf"` |
| 3.x | Link Object | `"operationRef"` | allowed, except `"operationId"` | treat the reference target as the target of the link described by the Link Object |
| 3.x | Discriminator Object | `"mapping"` | n/a | for each name under `"mapping"`, if the value is not the name of a Schema Object under the Components Object, treat it as a reference to the schema to use when the discriminator field matches the mapping name |
Copy link

Choose a reason for hiding this comment

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

Very informative. Just to check, for the 3.x labels these are all going to be unchanged in 3.2+ until moonwalk?

Copy link
Member Author

Choose a reason for hiding this comment

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

@notEthan thanks! They will remain compatible, more so than 3.0 to 3.1. I think we might deprecate some things like the special Path Item Object $ref behavior, but we won't remove anything.

@lornajane
Copy link
Collaborator

Thanks @handrews! I need more time to look at this properly, there are some really great nuggets in here of what doesn't work and why, but I think we might also add some "do it this way" advice and some examples to try to lead people out of the dark place in which they find themselves. Would it be okay for me to add changes directly to this pull request to restructure some of the content?

@handrews
Copy link
Member Author

@lornajane I'd had more content and guidance in a previous iteration of this, and had intended to add more in a later PR. But I'd very much welcome and appreciate collaboration on this. If it's easier for you to edit this PR in place, please go right ahead! I look forward to seeing the changes.

@handrews
Copy link
Member Author

@lornajane
Copy link
Collaborator

Thanks for being open to some content restructure @handrews - I shifted things out of FAQ format (not considered current practice in documentation, and not an established pattern in these docs) mostly because those articles can get long and difficult to navigate. The content is split into topics, with each page added to the sidebar.

In general, I think paragraphs might be easier to read than bullet points, but I suggest we don't block the pull request for that (I've done enough holding this one up already!).

I would have loved to have seen at least one or two examples in the text, I think that's something that the learn site can excel at - but we can add them later, maybe after hearing user feedback.

@handrews
Copy link
Member Author

@lornajane thanks! I will look at this in detail soon. Just to respond to a few comments first:

shifted things out of FAQ format (not considered current practice in documentation

Great! I'm not up on current best practices and was not attached to a FAQ format at all.

In general, I think paragraphs might be easier to read than bullet points

I've been told "bullet points, not paragraphs!" over and over by people who outright refuse to read anything I write in prose because I'm too wordy. I'm happy for anyone else to restructure it, I'm just hyper-sensitive at this point.

I would have loved to have seen at least one or two examples in the text

I am horrible at examples. I learn theory-first, unless the theory just isn't articulated and I have to construct it from examples. Most people learn examples-first. I've never understood how to connect with that, so hopefully others will fill them in. I'm happy to provide a topic list or feedback, though.

@lornajane
Copy link
Collaborator

Let's leave the bullet points and not worry too much about the examples. They can be fleshed out over time - I do think they're useful, but let's add them as we see them in the wild or something!

@lornajane lornajane self-requested a review February 24, 2024 19:59
Copy link
Collaborator

@lornajane lornajane left a comment

Choose a reason for hiding this comment

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

Approved, but I'll leave you time to take another look before one of us merges

@lornajane
Copy link
Collaborator

@handrews I think this is an improvement and we should merge if you're happy enough to let this version go as-is.

@handrews handrews merged commit 6779144 into OAI:main Mar 12, 2024
@handrews handrews deleted the refs branch April 22, 2024 16:20
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.

None yet

3 participants