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: adds a reference back to Subject from Sample #25

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

claymcleod
Copy link
Collaborator

@claymcleod claymcleod commented Nov 5, 2023

PR Close Date: November 17, 2023

Adds a reference back to the Subject from which a Sample was derived.

Considerations

  • I made this a required field, I'm not if it will hold true for all servers that every sample can be tied back to a subject reported on by that server. I do know it's true for us FWIW.
  • I considered a few other names, including parent, derived_from, and source, but ancestor seemed like the best choice. Let me know if anyone likes a different name better.

Before submitting this PR, please make sure:

  • You have added a few sentences describing the PR here.
  • You have added yourself or the appropriate individual as the assignee.
  • You have added the relevant groups/individuals to the reviewers.
  • Your commit messages conform to the Conventional Commits standard.
  • You have updated the README or other documentation to account for these changes (when appropriate).

@e-t-k
Copy link
Collaborator

e-t-k commented Nov 6, 2023

What about simply subject ? Would that be too confusing? (Or other technical reasons to avoid it?) I agree that ancestor is better than parent etc.

The Treehouse server will always report on a Subject for each Sample we serve, even if that Subject isn't originally ours (eg TARGET/ other 3rd party samples on which we've calculated gene expression); so that constraint will be fine for us as well.

LGTM in general though.

@claymcleod
Copy link
Collaborator Author

Great! Thanks for your comments. I do slightly like subject better, so I will update that and see what other commenters think.

@claymcleod
Copy link
Collaborator Author

Note to self: I will need to update this to use the new models::subject::Identifier when #27 is merged in.

@claymcleod claymcleod merged commit 6121881 into main Nov 17, 2023
5 checks passed
@claymcleod claymcleod deleted the feat/link-sample-to-subject branch November 17, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants