Skip to content

Support for specs version 1.0-draft #96

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

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

AbhishekPAnil
Copy link
Contributor

  • This PR includes updates to improve code readability and enhance schema support:
  • Refactored Candidate.js for cleaner logic and safer property access.
  • Renamed schema version "draft" to "1.0-draft".
  • Added lang and dir references in JSON schemas.
  • Standardized property names to camelCase.
  • Updated match logic.
  • Updates are also done as per the review in Enhancement/issue 13 #74

AbhishekPAnil and others added 30 commits April 24, 2024 11:46
…-post

Changed the reconciliation service request method to post,added the r…
…-post

Node engine version added to package json
@saumier saumier requested a review from wetneb June 13, 2025 16:46
@saumier
Copy link
Contributor

saumier commented Jun 13, 2025

@wetneb Hi. At our June W3C Reconciliation group meeting we discussed asking you to review this PR.

This PR is on a feature branch in the Culture Creates repo. It uses the manifest from the endpoint to determine which version of the API to use and supports the v1.0-draft match service on June 11 with minimal changes to the UI.

We have a reconciliation server running v1.0-draft at https://staging.recon.artsdata.ca (no SSL certificate). However our server will have breaking changes next week (specifically the renaming of v and pid updated in the spec June 12). This means you will probably not be able to test the testbench against our v1.0-draft service. You will still be able to test against v0.2 and the manifest version detection.

Let me know if you would like to proceed with the review. In the spirit of bringing our branches closer together I think it would be good to review and merge this current PR rather than wait. This will make the next PR easier to review :-)

@wetneb wetneb changed the title Task/issue 16 Support for specs version 1.0-draft Jun 14, 2025
@wetneb
Copy link
Member

wetneb commented Jun 14, 2025

Hi both, thanks for reopening this!

@AbhishekPAnil I have renamed this PR from "Task/issue 16" to "Support for specs version 1.0-draft". By doing that, I am trying to make it easier for people to understand what the PR is about when browsing the list of PRs in this repository. To link the PR to the issue it solves, you can add "Closes #16" to the description (not title!) of the PR. By this, I mean the body of your first comment in this discussion, which you should be able to edit. I would recommend that you follow this structure for any contributions you make in other GitHub projects too :)

I would like to go back to the discussion that I started in the previous PR, because I can't find your reactions to the suggestions I brought up there.

To me, the purpose of the test bench is to enable service developers to check that their service conforms to the specifications. So, it should be clear to the users which version of the specs they are testing their service for. At the moment you seem to select the version of the specs based on the manifest, but I am not clear on what happens when the manifest announces multiple versions. In such a scenario, I would ideally expect the test bench user to be able to select which version of the protocol to test for.

One simpler way of giving users this choice would be to deploy separate instances of the test bench: say, one for 0.1/0.2 (which are largely compatible) and one for 1.0. We could do that by making a first release from the current master branch, deploying that somewhere, and then adding further changes to the master branch which make it support only the 1.0-draft version.

I think this strategy is more viable than trying to support all versions of the specs in a single instance of the test bench, especially since the commit log of this PR is quite hard to follow (including some changes that seem irrelevant to this PR, and formatting changes that make it harder to understand what the functional changes are). In other words, this is not something I feel I am able to review as it stands.

I was able to test the PR interactively with your service and failed to execute a "Match" query, because it seems that the test bench is not making the corresponding POST request to the correct URL. It uses the manifest URL instead. So it does not seem to work.
image

So, my recommendation is:

  • to give up on trying to support multiple versions of the specs
  • make a release of the current repo, publish it at a dedicated URL (say reconciliation-api.github.io/testbench-v0.2/ for instance) for it to remain available there in the future
  • Just develop support for 1.0-draft in this repository directly. Do not to ask for my review, since the changes are too hard to follow: just push to the repository, which you can be the maintainers of :) Things will break and that's fine. You are in charge.

If you are interested in discussing how to make such PRs easier to review, maybe we could take some time at the next meeting to identify various independent changes in this PR that could be separated out, and how to use git's rebasing functionality to obtain a cleaner commit log.

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.

3 participants