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

feat: replace experimenter_full_name with Investigator class #1223

Conversation

dbirman
Copy link
Member

@dbirman dbirman commented Jan 9, 2025

This PR replaces all usage of *_full_name: str with investigator: List[Investigator] and introduces a new class Investigator. An Investigator can be defined either with a first and last name (and optional orcid id) or with an anonymous ID string.

@dbirman dbirman requested a review from saskiad January 9, 2025 17:51
Copy link
Collaborator

@saskiad saskiad left a comment

Choose a reason for hiding this comment

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

I'm just fundamentally torn on this. Is there a reason not to use the PIDName the way we do for Investigators in the data_description? If we do want a class for people, let's use a more generic term rather than experimenter.

@dbirman
Copy link
Member Author

dbirman commented Jan 14, 2025

I'm just fundamentally torn on this. Is there a reason not to use the PIDName the way we do for Investigators in the data_description? If we do want a class for people, let's use a more generic term rather than experimenter.

Oh I missed investigators, but also didn't we agree months ago to deprecate PIDName in favor of this exact kind of change where the registry is pre-populated for users?

@dbirman
Copy link
Member Author

dbirman commented Jan 21, 2025

Few more places where *_full_name pattern needs to be replaced

@dbirman dbirman requested a review from saskiad January 21, 2025 22:31
@dbirman dbirman changed the title feat: replace experimenter_full_name with Experimenter class feat: replace experimenter_full_name with Investigator class Jan 22, 2025
@@ -41,7 +41,7 @@ class DataDescription(DataCoreModel):
_DESCRIBED_BY_URL = DataCoreModel._DESCRIBED_BY_BASE_URL.default + "aind_data_schema/core/data_description.py"
describedBy: str = Field(default=_DESCRIBED_BY_URL, json_schema_extra={"const": _DESCRIBED_BY_URL})
schema_version: SkipValidation[Literal["1.0.4"]] = Field(default="1.0.4")
license: Literal["CC-BY-4.0"] = Field("CC-BY-4.0", title="License")
license: Literal["CC-BY-4.0"] = Field(default="CC-BY-4.0", title="License")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to have a default for a literal? I thought the point of a literal is to have a fixed value. (This is a question, not a review ... )

Copy link
Member Author

Choose a reason for hiding this comment

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

Not related to the literal (and this probably shouldn't be in this PR, I'll remove it). Some linters seem to struggle with interpreting the default value as the default unless you set it explicitly, so when you try to instantiate a DataDescription object the linter complains that the license field is required

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's odd. we should see if Jon has thoughts on this. I'm not opposed to having the default, I just was surprised by it.

@saskiad saskiad merged commit 7349ad6 into release-v2.0.0 Jan 24, 2025
6 checks passed
@saskiad saskiad deleted the 1175-20-replace-field_full_name-usage-with-a-class-researchername branch January 24, 2025 06:04
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