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

0.9.4 pre-release tasks #459

Merged
merged 42 commits into from Dec 7, 2023
Merged

0.9.4 pre-release tasks #459

merged 42 commits into from Dec 7, 2023

Conversation

duncandewhurst
Copy link
Contributor

@duncandewhurst duncandewhurst commented Dec 4, 2023

Related issues

Description

512eb18 adds to manage.py the script that I used to link field names to the schema reference tables. I thought that it might come in handy as it could be run against other Markdown content in the standard, but I'm happy to remove it if it's not going to be useful or if it's too much work to get the code up to the necessary standard.

Merge checklist

If there are changes to project-schema.json or project-package-schema.json:

  • Update the examples:
    • docs/examples/example.json
    • docs/examples/blank.json
  • Run ./manage.py pre-commit to update docs/_static/i8n.csv

If you added a new definition to the schema, run ./manage.py pre-commit.

If you added a new codelist:

  • Add an entry to docs/reference/codelists.md

Regex find: `([a-zA-Z.]*)` object
Regex replace: [`$1`](../../reference/schema.md#\l$1) object
@duncandewhurst duncandewhurst marked this pull request as ready for review December 5, 2023 04:01
@duncandewhurst
Copy link
Contributor Author

@odscjen for the changes to sustainability.yaml and sustainability.md, I think the best thing to do would be to review the HTML version. For the changes to project-schema.json, best to review 4fbfb32 separately and just check that the order matches that specified in #423.

Copy link
Contributor

@odscjen odscjen left a comment

Choose a reason for hiding this comment

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

It looks like lots but they're mostly very minor copy edits in sustainability.yaml and common.md and they mostly fall into the following main categories:

  1. adding a full stop at the end of a step.
  2. rearranging the mapping so that it always starts with the stage (unless it can be either) and then a line between that and the actual instructions, I added those in for visual consistency.
  3. removing full stops from in front of top level fields, e.g. .parties should be parties, or where the Contracting process has already been specified as the level then .summary changed to summary.

A couple of other things I noticed:

  • There's a few places where it says "Project-level" instead of "Project level" but I couldn't put it in as a suggestion as it's not from this PR. I think I noted them all but I might have missed some.
  • Lots of the "disclosure format" cells that are pulling in from the CoST spreadsheet are missing bullet points so they don't look like proper lists, e.g. Institutional policy coherence.

docs/cost/common.md Outdated Show resolved Hide resolved
docs/cost/common.md Outdated Show resolved Hide resolved
docs/cost/common.md Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
mapping/sustainability.yaml Outdated Show resolved Hide resolved
docs/cost/common.md Outdated Show resolved Hide resolved
@duncandewhurst
Copy link
Contributor Author

  • There's a few places where it says "Project-level" instead of "Project level" but I couldn't put it in as a suggestion as it's not from this PR. I think I noted them all but I might have missed some.

Fixed in 73bc7ab

  • Lots of the "disclosure format" cells that are pulling in from the CoST spreadsheet are missing bullet points so they don't look like proper lists, e.g. Institutional policy coherence.

Fixed in e3babaa

@duncandewhurst
Copy link
Contributor Author

@jpmckinney a few suggestions for your review:

512eb18 adds to manage.py the script that I used to link field names to the schema reference tables. I thought that it might come in handy as it could be run against other Markdown content in the standard, but I'm happy to remove it if it's not going to be useful or if it's too much work to get the code up to the necessary standard.

There are two commits with large diffs that may be best to review separately:

  • 4fbfb32 reorders the top-level fields
  • 10f403e links field names in sustainability.yaml to the schema reference tables (no worries if you're not concerned about reviewing sustainability.yaml- @odscjen did a detailed review already).

- Add Extension column to Finance codelists
- Correct encoding of currency
- Small modifications to documentType, milestoneCode, milestoneType codes
- Small modifications to schema descriptions
- Change schema property order
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.

Please review the changelog:

  • The changelog for project-schema.json: deprecate budget.budgetBreakdown, add budget.bud… #450 uses "deprecate" but I think it was simply renamed (which is fine as we're not 1.0).
  • On that note, I'm not sure that we need to deprecate 'completed' – we can just remove it (i.e. rename it).
  • Some new fields seem to be missing, e.g. policyAlignment. I haven't checked codes.
  • We can probably add some text at the top describing the major change, but we can also do this in a new PR.

I ended up doing a major refactor of manage.py. The update and pre-commit commands still work (they are the only ones I know how to use). I didn't touch the two update-sustainability commands. You'll have to test lint (there are no ambiguous unlinked fields to test against).

Note: You need to run ./manage.py update --ppp-base-url https://raw.githubusercontent.com/open-contracting-extensions/public-private-partnerships/1.0-dev/schema/patched/ when using the update command.

Otherwise, this PR looks good!

Edit: One of my commits re-ran update which did make minor changes.

@jpmckinney
Copy link
Member

Ah, milestoneType has some unwanted edits in c83ff80

@duncandewhurst
Copy link
Contributor Author

Thanks for the changelog tidy and manage.py refactor! All of your changes look good to me.

* The changelog for [project-schema.json: deprecate budget.budgetBreakdown, add budget.bud… #450](https://github.com/open-contracting/infrastructure/pull/450) uses "deprecate" but I think it was simply renamed (which is fine as we're not 1.0).

Done in b1ed578. I used 'replace' rather than 'rename' since the field's items also changed.

* On that note, I'm not sure that we need to deprecate 'completed' – we can just remove it (i.e. rename it).

Done in d36e142

* Some new fields seem to be missing, e.g. `policyAlignment`. I haven't checked codes.

Fixed in 4d9b86c. I checked the new fields and I think only policyAlignment was missing. I also moved the entry for budget.description to the list of added fields.

I've checked codelists and codes and added the missing ones in 6f7554d

* We can probably add some text at the top describing the major change, but we can also do this in a new PR

A new PR sounds good.

I ended up doing a major refactor of manage.py. The update and pre-commit commands still work (they are the only ones I know how to use). I didn't touch the two update-sustainability commands. You'll have to test lint (there are no ambiguous unlinked fields to test against).

Confirmed that the update-sustainability commands still work. lint -l was broken, but I've fixed it in dc86a52.

Edit: One of my commits re-ran update which did make minor changes.

Changes look good, aside from:

Ah, milestoneType has some unwanted edits in c83ff80

Reverted in 08ed35f

@duncandewhurst duncandewhurst merged commit d97e009 into 0.9-dev Dec 7, 2023
10 checks passed
@duncandewhurst duncandewhurst deleted the 423-pre-release-tasks branch December 7, 2023 23:33
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.

OC4IDS 0.9.4: Pre-release tasks
3 participants