-
Notifications
You must be signed in to change notification settings - Fork 21
Add w3curie type and edit def of curie to be less restrictive #221
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
base: main
Are you sure you want to change the base?
Conversation
@cmungall @sujaypatil96 and anyone else who is interested -- please add any comments as it'd be great to get this merged! |
LGTM. We could then adapt some of the validations to be strict on w3curie. |
Would it be possible to have something like w3safecurie too as a subtype of curie? That way we could have something like URIorSafeCURIE too. [Update]: thanks @dalito for the link |
In this repo I cannot help with approving or merging but I also support Silvano´s request for the other CURIE Datatypes. |
I don't have merge permissions -- @cmungall @sierra-moxon can you approve/merge? |
@ialarmedalien I can merge, but the (undocumented) policy says that only the core-team is allowed to merge. And I'm not part of it. I started a discussion to get this kind of policies, workflows and processes documented. But up to now I didn't get any resonance in the community 🤷 |
I'm with you on the lack of clarity on permissions that you talk about in your discussion! I'd guess there hasn't been much response to the issue because (a) discussions are hidden away from the main issue tracker / PRs, and (b) it's a very difficult question to answer... |
Then vote it up 😉
That's why I raise the point whenever I find a fitting situation within the scope of a PR or issue, like in this case. Additionally, people give "thumbs up" instead of "votes". And discussion lists show votes, but not emojis 🤷 |
@Silvanoc good point about "thumbs up" vs. "votes". I think for a thumbs-up to count as a vote, it has to be on the issue description (i.e., the first "comment", as opposed to any subsequent comments) - is that correct? Regarding this PR, I realize you are eager to get it merged. However, we are extra-careful about changes that affect the metamodel. This PR has no automated tests to make sure it doesn't break anything else, and it's also not clear whether anyone manually tested how this change affects things when imported into an existing LinkML model repository (such as Biolink) or any downstream repo in the LinkML ecosystem. To avoid potential problems caused by merging this PR, we need people who are deeply familiar with the metamodel and the whole codebase to review it. (This would of course be easier if there were tests included.) I hear you on the frustration about lack of clarity in the process for reviewing and merging PRs. This is something that many open-source projects run into as they grow. Thank you for opening Discussion #2518 and including examples of some other open-source projects' contribution protocols. We will definitely look into those. |
@nlharris Not sure what kind of tests should be added -- the PR registers a new data type, derived from existing data types, to the data model; it does not add any functions or methods, which would require tests. There are no extant tests for this part of the model that new tests could be modelled on. I'm happy to add some if you want to suggest what they should do. |
Apparently "thumbs up" are never counted as "votes" in the discussions view. Discussion #2518 has 3 votes (one of them mine) and 3 thumb-ups (none of them mine), so "thumb-up" =/= "vote". And the discussion view does not add them up, but only show the votes (3).
I agree with @ialarmedalien that there is probably no need for any tests to be added. Being a derived type, tests applicable to the type parent applies also here. Once we extend validation to really differentiate this subtype from its parent, we will need tests.
OSS projects typically start opening the source code, but are not open communities. At that point there might not be a real need to processes, as long as the community knows some otherwise established or common processes. But OSS projects eventually become also open communities. And at that point openly documented processes become important for a smooth community interaction. IMO LinkML has reached that point for a long time now and should really document at least the basics. |
Agree completely. As @Silvanoc alluded to in an earlier comment, there seems to be a "core" LinkML developer team, but it's not clear who is part of that team beyond Chris M and Sierra. Having established and documented procedures for PR triage would also be beneficial for maintaining positive sentiment in the community. |
@ialarmedalien - I understand from this PR that the new type is wanted and warranted. And I like the new type. Has this new type been incorporated into local linkml-runtime and linkml branches, and run through the tests in those repos? From the tests in this repo: As a core dev, my next steps with this PR would be to:
I'll get started on this, I've added myself as a reviewer. |
Fixes linkml/linkml#258.
Adding in the new w3curie type.
I tried to update the repo to use py3.12 with the current versions of linkml, linkml-runtime, and pyyaml so that I could regenerate the project files. Run
make gen-project
gave the following output:Looks like the various project generators, etc., have become more restrictive since
linkml-model
was last updated.ETA: just seen the series of PRs to update the model repo. I can rebase this PR and generate the new files once the deps and other bits and pieces are fixed and merged.