Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Scope, Cardinality, Type, and DictionaryItem classes #6938
Add Scope, Cardinality, Type, and DictionaryItem classes #6938
Changes from all commits
561027b
098c30a
43b0aba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like the example is a bit too specific, why not use something like
For instance, site affiliations for a user
for us non-imaging peopleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind changing the example but I think site/project affiliations is a bad example because there's no "user" scoped data, and the example might get confusing since site/project affiliations exist on both candidate and session scopes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about using the candidate/session relationship then ? a candidate can have 0 to many sessions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be confusing too though, because there's the session scope and there's no variable named "Session", and session based data is usually described as 1:1 with a session scope, not 1:many with the candidate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why single letter variables, i'm not a big fan and in the long run they always tend to be a hindrance (same goes for shortcuts like
cand
forcandidate
...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just the argument name in the constructor, the context only lasts long enough to assign it to a property right below.. the property name is the full word.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems limiting to projects wanting to define their own scopes... why not a database table defining the constants...
would avoid overriding/overloading specially since we dont offer any easy overriding structure for the
src
directoryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scopes can't be extensible because everywhere in the code that deals with the data needs to understand how to deal with that scope. Let's say a module added a scope named "foobar" and someone accessed that a variable with that scope in the DQT. Questions like "how should this be displayed? How does it relate back to candidates? How does it relate to sessions? Does it need to show a list of sessions to select? How does it get displayed in the results table? How do I join it with candidate scoped data to show in a row?" need to be understood by the code to build the interface and display results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if you have a suggestion for how to make the comment more clear I'll update it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no your right... I'm just thinking if there is a way we can add project defined scopes in the same way. my immediate interest is the Biospecimen scope for CBIGR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could come later though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are similar problems with things like imaging QC. "Pass/Fail" is really file scoped data but there is no file scope. For now I've defined them as "many" cardinality scoped to the session.