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

Definition namespaces and providers #473

Merged
merged 9 commits into from
Mar 21, 2024
Merged

Definition namespaces and providers #473

merged 9 commits into from
Mar 21, 2024

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jun 14, 2023

This PR begins the work on #328, adding prefixes that can be shared across providers. I will try to follow this rough plan with my work:

  • Define "definition provider" [working title]
  • Replace relevant mentions of "database-provider-specific" with "namespace-specific"
  • Potentially extend the provider/link definition to add a specialism for this kind of provider (decided against it)
  • Describe mechanism for becoming a definition provider
  • Provide example cases

Then outside of the specification we need to:

  • Set up an example definition provider, as a separate repository, that can be modified for future domain-specific extensions

Closes #328

.words.lst Show resolved Hide resolved
@ml-evs ml-evs marked this pull request as ready for review July 3, 2023 22:50
@ml-evs
Copy link
Member Author

ml-evs commented Jul 3, 2023

I think this is ready for some initial feedback. I've also made the start of a namespace repo at https://github.com/ml-evs/optimade-stability-namespace (currently forked to https://github.com/Materials-Consortia/optimade-stability-namespace) where I am trying to use @rartino's processing tools from #445 to generate the website/schemas (currently with no luck).

@ml-evs ml-evs marked this pull request as draft July 3, 2023 22:51
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs changed the title [WIP] Definition namespaces and providers Definition namespaces and providers Aug 25, 2023
optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs marked this pull request as ready for review December 19, 2023 20:26
optimade.rst Outdated Show resolved Hide resolved
@rartino
Copy link
Contributor

rartino commented Jan 9, 2024

@ml-evs I've gone through this, and aside the minor formatting issue @merkys remarked on (and I've provided a diff for), this looks very good to me and I don't quite see why we cannot already merge it.

The new optimade-property-tools repo with its example directory may make it a bit easier to run a namespace provider test.

@rartino rartino added the PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR label Jan 10, 2024
@rartino rartino mentioned this pull request Jan 10, 2024
optimade.rst Outdated Show resolved Hide resolved
@ml-evs ml-evs requested a review from merkys January 10, 2024 22:01
@ml-evs
Copy link
Member Author

ml-evs commented Jan 10, 2024

Thanks @rartino, happy to merge this before rc.2 so we can test out the implications.

.words.lst Outdated Show resolved Hide resolved
@ml-evs ml-evs added this to the v1.2 milestone Jan 17, 2024
@ml-evs ml-evs added PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing and removed PR/waiting-for-update This PR has been reviewed and is waiting for the author to response or update the PR labels Jan 17, 2024
rartino
rartino previously approved these changes Feb 9, 2024
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

This looks good to me. We are already trying this out in other repos.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Outdated Show resolved Hide resolved
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

I made some minor suggestions to put definition-provider prefixes to the same level as database-provider prefixes. AFAIK they should be roughly equivalent from now.

optimade.rst Outdated Show resolved Hide resolved
optimade.rst Show resolved Hide resolved
@ml-evs ml-evs added the blocking-release This is a PR or issue that presently blocks the release of next version of the spec. label Mar 15, 2024
@ml-evs ml-evs requested review from merkys and rartino March 17, 2024 18:12
merkys
merkys previously approved these changes Mar 18, 2024
Copy link
Member

@merkys merkys left a comment

Choose a reason for hiding this comment

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

Good to go, thanks!

rartino
rartino previously approved these changes Mar 18, 2024
Copy link
Contributor

@rartino rartino left a comment

Choose a reason for hiding this comment

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

Re-confirming my positive approval.

ml-evs and others added 7 commits March 18, 2024 21:49
Co-authored-by: Antanas Vaitkus <antanas.vaitkus90@gmail.com>
Co-authored-by: Rickard Armiento <gitcommits@armiento.net>
Co-authored-by: Andrius Merkys <andrius.merkys@gmail.com>
@ml-evs ml-evs dismissed stale reviews from merkys and rartino via 18bc65e March 20, 2024 11:57
@ml-evs
Copy link
Member Author

ml-evs commented Mar 20, 2024

Ok, now that the one linting issue is fixed (with the checker introduced since your last reviews...) I think this is the final time I need to bother you all with review requests (on this PR...)

@ml-evs ml-evs requested review from rartino and merkys March 20, 2024 18:34
@ml-evs ml-evs merged commit 9852fcc into develop Mar 21, 2024
5 checks passed
@ml-evs ml-evs deleted the ml-evs/namespaces branch March 21, 2024 09:33
@merkys merkys mentioned this pull request Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocking-release This is a PR or issue that presently blocks the release of next version of the spec. PR/ready-for-review Add this flag if you are the author of the PR and you want it to be reviewed. Remove it when editing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prefixes not directly connected to databases
4 participants