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

1449 improve examples #1661

Merged
merged 43 commits into from
Mar 28, 2024
Merged

1449 improve examples #1661

merged 43 commits into from
Mar 28, 2024

Conversation

odscjen
Copy link
Contributor

@odscjen odscjen commented Nov 20, 2023

Addresses #1449

Added realworld example to primer/how.md taking altered version from Ebonyi State as agreed in the issue.

Swapped out fictional example in guidance/build/merging.md for a corrected version of a real example from Ghana.

Added reference to merging and change history examples to primer/release_records.md.

Converted example in guidance/map/buyers_suppliers.md to JSON from csv as agreed.

Haven't done anything to the Identifiers section as it is being dealt with separately.

@odscjen
Copy link
Contributor Author

odscjen commented Nov 28, 2023

@jpmckinney in updating the examples I'm getting the above test errors which appear to be looking for a specific date which was in the fictional example but not in the new real-world example. Could you please update the test to remove the need to this particular date to appear, unless there is a reason why it's needed?

@jpmckinney
Copy link
Member

jpmckinney commented Nov 28, 2023

This test is one you can edit in this repository at test_docs.py. The test is checking whether the dropdown used to switch between examples works. It tries to find text that appears in one example but not the other. You can use any string within those examples that isn't expected to change in the future.

@odscjen odscjen marked this pull request as ready for review November 30, 2023 16:48
docs/primer/how.md Outdated Show resolved Hide resolved
docs/primer/how.md Outdated Show resolved Hide resolved
docs/primer/how.md Outdated Show resolved Hide resolved
docs/primer/how.md Outdated Show resolved Hide resolved
docs/primer/releases_and_records.md Outdated Show resolved Hide resolved
docs/guidance/build/merging.md Outdated Show resolved Hide resolved
docs/guidance/build/merging.md Outdated Show resolved Hide resolved
docs/guidance/map/buyers_suppliers.md Outdated Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
@duncandewhurst
Copy link
Contributor

@jpmckinney I'm interested to know what you think of my suggestions in #1661 (comment), #1661 (comment) and #1661 (comment). After reviewing this PR, I think that most of the JSON files featured in (existing) examples in the OCDS documentation are way too long and would better serve readers' needs if we cut the fields right down to those needed to illustrate the specific examples that they feature in. That would also make the examples much easier to maintain when the schema changes.

@jpmckinney
Copy link
Member

jpmckinney commented Dec 1, 2023

Yes, we should have minimal (but coherent) examples.

The purpose of #1449 is to avoid examples that are incoherent, or that are too abstract (e.g. Anytown buys widgets).

The means we agreed on was to base examples on real data.

That said, we don't care at all about matching a real existing release, as expressed in #1449 (comment)

@duncandewhurst
Copy link
Contributor

Great. Shall we expand the scope of this PR to cover making the examples minimal?

@jpmckinney
Copy link
Member

Yes, let's do it in one shot.

@jpmckinney
Copy link
Member

jpmckinney commented Dec 1, 2023

Or do you mean examples not already edited by this PR? I think let's just get the examples touched by this PR done.

We can open a new PR or issue for others.

@odscjen
Copy link
Contributor Author

odscjen commented Dec 1, 2023

@duncandewhurst I've cut down the examples a lot, let me know if you think even more could be trimmed without making the examples feel fake

@duncandewhurst
Copy link
Contributor

@jpmckinney this PR is ready for your review. I've updated the examples it touches in keeping with the discussion in #1449 and #1666. I'll tackle the remainder of the examples as a part of #1666.

Copy link
Member

@jpmckinney jpmckinney left a comment

Choose a reason for hiding this comment

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

Okay to merge if all suggestions merged and comments resolved.

Note that some suggestions might be hidden by GitHub and need to be expanded.

docs/guidance/build/merging.md Outdated Show resolved Hide resolved
docs/guidance/map/amendments.md Outdated Show resolved Hide resolved
docs/guidance/map/amendments.md Outdated Show resolved Hide resolved
docs/guidance/map/amendments.md Outdated Show resolved Hide resolved
docs/guidance/map/amendments.md Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
docs/schema/merging.md Outdated Show resolved Hide resolved
docs/examples/organizations/consortia_simple.json Outdated Show resolved Hide resolved
@jpmckinney
Copy link
Member

jpmckinney commented Mar 27, 2024

We'll use "Squash and merge", since the commits in this PR went back and forth a bit.

@jpmckinney
Copy link
Member

I think docs/examples/mergin/updates/merged.json is unused. It can be deleted, as well its mention in READMEmd.

I think docs/examples/change_history/tenderAmendment.json is also unused and can be deleted.

docs/examples/merging/updates/versioned.json needs to be regenerated per the readme. It's used on a couple pages, where I think the directives are fine as-is.

@duncandewhurst
Copy link
Contributor

I think docs/examples/mergin/updates/merged.json is unused. It can be deleted, as well its mention in READMEmd.

I think docs/examples/change_history/tenderAmendment.json is also unused and can be deleted.

Both removed in fa9161a

docs/examples/merging/updates/versioned.json needs to be regenerated per the readme. It's used on a couple pages, where I think the directives are fine as-is.

The 'source' files no longer exist so I've instead updated the directives to reference docs/examples/amendments/tender.json, created a linked release version of the same, and made one minor update to the accompanying text.

@duncandewhurst
Copy link
Contributor

@jpmckinney since my update above differs slightly from your suggestion, please let me know if you're happy for me to squash and merge.

@jpmckinney jpmckinney merged commit b93ef71 into 1.2-dev Mar 28, 2024
10 checks passed
@jpmckinney jpmckinney deleted the 1449-improve-examples branch March 28, 2024 01:36
@jpmckinney jpmckinney mentioned this pull request Mar 28, 2024
1 task
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