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

Add SPARQL to find uses of deprecated DC #817

Merged
merged 7 commits into from
Oct 4, 2023

Conversation

cthoyt
Copy link
Contributor

@cthoyt cthoyt commented Mar 1, 2023

This is a follow-up to oborel/obo-relations#692

@matentzn matentzn requested a review from anitacaron March 1, 2023 13:57
Copy link
Contributor

@anitacaron anitacaron left a comment

Choose a reason for hiding this comment

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

Actually, we moved all sparql queries to the jinja template. You can add after this line following the pattern from line 602

@matentzn
Copy link
Contributor

matentzn commented Mar 1, 2023

Oops my bad. Dont worry @cthoyt I can do it next time I do an ODK push if you don't want to do it!

@anitacaron anitacaron self-assigned this Sep 27, 2023
@anitacaron anitacaron added this to the 1.5 milestone Sep 27, 2023
@anitacaron
Copy link
Contributor

I added this QC as default.

Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

I think this check may be a bit to extreme for a default check. This will break a lot of ontologies out there..

At the very least, the check needs to be constrained to classes from the base namespace, but even then, I would feel safer if, instead of messing with the entire namespace at once, we enumerate the specific properties we want to standardise explicitly, like creator, contributor, license and description. What do you think?

@cthoyt
Copy link
Contributor Author

cthoyt commented Sep 28, 2023

@matentzn fair enough, but it's also the case that this would only apply to anyone creating a new ontology from now, right? And it's reasonable to ask people starting now to follow higher standards.

Though, I do agree that we should slice the check to only apply to terms from the current ontology. Ontology developers aren't typically proactively contributing back to improve upstream ontologies they import. But, I'm not really sure if the impact of choosing one way or another is a really big deal. From the ontologies I've upgraded from DCE to DCTERMS, there have been only a small number of fixes necessary, and never from imported triples

@anitacaron anitacaron self-requested a review September 29, 2023 08:51
@matentzn
Copy link
Contributor

would only apply to anyone creating a new ontology from now, right

No, if someone updates to the latest ODK, and they have not configured a custom checklist, they will get this new test..

I guess I would like at the very least to restrict it to classes from the same ontology. I would still prefer to enumerate the properties that we want to exclude from dce explicitly (as a nice byproduct, the test will become much faster), but if one more person from the ODK developers team argues against me I will falter and agree with the slightly more radical option to get rid of dce altogether.

@anitacaron anitacaron requested review from matentzn and removed request for anitacaron October 2, 2023 19:14
Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

I assume you tested this - it looks good to me but I didn't try it!

template/_dynamic_files.jinja2 Show resolved Hide resolved
@cthoyt
Copy link
Contributor Author

cthoyt commented Oct 3, 2023

@matentzn @anitacaron this is cool, but like many things in this ecosystem, I think it has become so complicated that nobody except you two can make a contribution :/

@matentzn
Copy link
Contributor

matentzn commented Oct 3, 2023

We are working on a way to replacing all of the QC system with a much more simple approach. We are even thinking about doing this soon. For now, you will have to collaborate with is if you want to know how to get these kinds of fixes in.. This PR does not even seem that bad. What specifically fo you find complicated?

@anitacaron anitacaron merged commit 8dd8879 into INCATools:master Oct 4, 2023
1 check passed
@cthoyt cthoyt deleted the patch-2 branch October 4, 2023 15:35
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