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

Type model and API rework #28

Merged
merged 9 commits into from
Aug 17, 2023
Merged

Type model and API rework #28

merged 9 commits into from
Aug 17, 2023

Conversation

kMutagene
Copy link
Member

This PR:

  • Re-models Cvterm and CvUnit as records
  • uses unified naming of CvTerm/CvUnit record fields across all functions:
    • Accession instead of id or AnnotationId
    • Value instead of name
    • RefUri (instead of ref)
  • adds many convenience and accession functions on the modules/types that are actually used in the functions. Example: tryCvParam, a function of signature IParamBase -> CvParam option will not be on the CvParam type anymore, as functions prefixed with CvParam are expected to have actual CvParams as arguments.

@omaus
Copy link
Collaborator

omaus commented Aug 16, 2023

Value instead of name

is "Value" not already taken by the real value?

I mean, you have a CvParam with

  • TAN "TO:000000"
  • Ref "TO"
  • Name "Total traffic load"
  • Value of string "563 KB"

@kMutagene
Copy link
Member Author

kMutagene commented Aug 16, 2023

Value instead of name

is "Value" not already taken by the real value?

I mean, you have a CvParam with

  • TAN "TO:000000"
  • Ref "TO"
  • Name "Total traffic load"
  • Value of string "563 KB"

I am talking about CvTerm, not CvParam.

@kMutagene
Copy link
Member Author

kMutagene commented Aug 16, 2023

We can however decide to use Name if that is more favorable. the thing is we had always 2 choices, and they were used inconsistently across the library, as CvTerm was described as:

/// Represents a term from a controlled vocabulary (Cv)
/// in the form of: id|accession ; name|value ; refUri
// ?Maybe []

@kMutagene
Copy link
Member Author

kMutagene commented Aug 16, 2023

Also, to be clear, what you are referring to as value is the ParamValue.

@omaus
Copy link
Collaborator

omaus commented Aug 16, 2023

We can however decide to use Name if that is more favorable. the thing is we had always 2 choices, and they were used inconsistently across the library, as CvTerm was described as:

/// Represents a term from a controlled vocabulary (Cv)
/// in the form of: id|accession ; name|value ; refUri
// ?Maybe []

Yes, and I didn't like it.

Wouldn't it be more consistent when using "name"? Because if "value" in CvParam means a totally other thing than "value" in CvTerm, wouldn't this confuse people?

@kMutagene
Copy link
Member Author

kMutagene commented Aug 16, 2023

Is there a case where the Name/Value field represents something else than the name of an ontology term? if not I think we can use Name everywhere. Otherwise, I think Value is more generic and therefore better.

@omaus
Copy link
Collaborator

omaus commented Aug 16, 2023

Is there a case where the Name/Value field represents something else than the name of an ontology term? if not I think we can use Name everywhere. If not, I think Value is more generic and therefore better.

OBO format goes with "name" for Term names which makes it more clear imho.
Besides that, I don't understand your double "if not" case. 😅

@kMutagene kMutagene marked this pull request as ready for review August 17, 2023 09:31
@kMutagene
Copy link
Member Author

ready for review @omaus ;)

@kMutagene kMutagene merged commit 04f5158 into main Aug 17, 2023
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.

2 participants