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 curie_map to the model #376

Merged
merged 10 commits into from
Aug 6, 2024
Merged

Add curie_map to the model #376

merged 10 commits into from
Aug 6, 2024

Conversation

gouttegd
Copy link
Contributor

@gouttegd gouttegd commented Jul 24, 2024

Resolves #225

  • docs/ have been added/updated if necessary
  • make test has been run locally
  • tests have been added/updated (not applicable – nothing new to test, the CURIE map has existed for a while, this PR just formalises it directly into the model)
  • CHANGELOG.md has been updated.

If you are proposing a change to the SSSOM metadata model, you must

  • provide a full, working and valid example in examples/ (no need for a new example – all existing examples already have a curie_map)
  • provide a link to the related GitHub issue in the see_also field of the linkml model
  • provide a link to a valid example in the see_also field of the linkml model (no need, see above)
  • run SSSOM-Py test suite against the updated model

This PR makes the curie_map a bona fide slot of the MappingSet class, that exists independently of the serialisation format.

It builds on top of #349 by explaining in the text version of the spec what the curie_map slot is for, and how it should be dealt with in the SSSOM/TSV serialisation.

matentzn and others added 5 commits February 6, 2024 12:35
Now that the SSSOM model formally includes a `curie_map` slot, we can
amend the spec to:

1. describe the purpose of that slot in the general description of the
   model;

2. rephrase the section about that slot in the specification of the
   SSSOM/TSV format, to remove the notion that this slot is a
   "supplementary slot" that is specific to the TSV format.
@gouttegd gouttegd self-assigned this Jul 24, 2024
@gouttegd gouttegd requested a review from matentzn July 24, 2024 11:43
@matentzn matentzn mentioned this pull request Aug 3, 2024
8 tasks
Copy link
Collaborator

@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 added the "example" after all, just for completeness reasons. One small comment, then we can merge.

src/docs/spec-formats-tsv.md Show resolved Hide resolved
@matentzn
Copy link
Collaborator

matentzn commented Aug 3, 2024

Awesome thank you!!!

matentzn
matentzn previously approved these changes Aug 3, 2024
Copy link
Collaborator

@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.

Looks great, thank you!

@matentzn matentzn requested review from ehartley and hrshdhgd August 3, 2024 10:38
While the single value is allowed, we want to discourage it. Thanks @gouttegd for the note about this!
matentzn
matentzn previously approved these changes Aug 3, 2024
@matentzn matentzn added the schema label Aug 4, 2024
Copy link
Contributor

@hrshdhgd hrshdhgd left a comment

Choose a reason for hiding this comment

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

Sorry to repeat this in every PR

run make all same as #386 (review)

@gouttegd
Copy link
Contributor Author

gouttegd commented Aug 5, 2024

Sorry to repeat this in every PR

No need to be sorry. :) However:

  1. Re-generating and committing all derived files upon every PR is, I believe, a very bad idea.
  • It makes even the slightest PR much larger than it needs to be, and therefore more cumbersome to review.
  • It pollutes the history of the project with large changes that do not mean anything (only the changes to the source schema are actually meaningful).
  • It greatly increases the likelihood of merge conflicts when there are several PRs in the works.

It would be much better in my opinion to leave the auto-generated files untouched most of the times, and only commit them once in a while (typically shortly before a release).

  1. If you insist on having the derived files re-generated upon every PR, then it‘s something that should be mentioned in the CONTRIBUTING.md file, which for now has no trace at all of such of requirement – instead it’s full of useless, generic guidelines that were mindlessly copied over from another project.

And since there is a PR template, that requirement should also be mentioned in it.

hrshdhgd
hrshdhgd previously approved these changes Aug 5, 2024
@gouttegd
Copy link
Contributor Author

gouttegd commented Aug 6, 2024

I‘ll fix the conflict here once #375 will have been (re-)approved ­– no point in fixing it now since merging #375 will (again) cause a conflict in the changelog.

Copy link
Collaborator

@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.

Looks great, thank you!

@matentzn matentzn requested a review from hrshdhgd August 6, 2024 18:14
@gouttegd gouttegd merged commit 81fe996 into master Aug 6, 2024
3 checks passed
@gouttegd gouttegd deleted the add-curie-map-2 branch August 6, 2024 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New metadata element]: MappingSet.curie_map
3 participants