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

Clarify 'or' requirement in message for subject age + DOB #411

Merged
merged 13 commits into from
Oct 26, 2023

Conversation

CodyCBakerPhD
Copy link
Contributor

We discovered this during an alpha test and it caused some confusion

DANDI only requires one or the other; theoretically possible to have both, but the message implies it's required to have both, which is not true

@garrettmflynn
Copy link
Member

While a better direction for users of the GUIDE, it's technically incorrect—as the metadata is missing age AND date of birth.

Would we be able to note Subject requires an age or date of birth. instead?

Also, the reason it's confusing on the GUIDE is that only one of the fields (age) is checked AND that field provides information about both values. Would it be out of the question to expose the two checks separately—possibly combining them for an aggregate check that still behaves like this one?

@CodyCBakerPhD
Copy link
Contributor Author

While a better direction for users of the GUIDE, it's technically incorrect—as the metadata is missing age AND date of birth.

I've modified the logic to reflect the cases

Would we be able to note Subject requires an age or date of birth. instead?

That is what is says now, right?

Would it be out of the question to expose the two checks separately

The check applies to the Subject object entirely, and via PyNWB the form of date_of_birth is already checked for format so no need for an extra check function

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) October 26, 2023 15:17
@garrettmflynn
Copy link
Member

garrettmflynn commented Oct 26, 2023

That is what is says now, right?

I guess when I read "Subject is missing age or date_of_birth.", I think one is provided but the other is not. But this PR was raised because "Subject is missing age and date_of_birth." implies that both are / could be required—though it is correctly describing the Subject metadata.

What both of these messages fail to accomplish is a clear indication of the user's course of action based on the requirements. So I was thinking a message that directly states "Subject requires an age or date of birth." is a better way to describe what the user should do (provide one or the other) rather than merely describe the status of the fields being evaluated by the Inspector.

@CodyCBakerPhD
Copy link
Contributor Author

Except the word 'requirement' in the message is only true with the DANDI config, not in general (anyone can make a Subject object without anything more than an ID) - and making config-dependent error messages would be more complicated than simply correcting the wordage here

a better way to describe what the user should do (provide one or the other) rather than merely describe the status of the fields being evaluated by the Inspector.

The phrasing across the Inspector is consistently the latter; it only reports what is missing, or what structures are written incorrectly; occasionally it will mention how to fix it, but for existence checks I'd it is pretty self-evident that if the Inspector says something is missing, the way to get it to dismiss the message is to make it not missing

In this special case there are two things that are each missing in order to trigger the message, and the incorrect message is to say that both are missing and therefore both need to be specified to make the message go away

Anyway, I added an extra clarification to the message without making it seem like a hard requirement

@garrettmflynn
Copy link
Member

Ah thank you for the clarification about the difference between when these requirements hold.

With your clarification, though, I think it's clearer to keep the and over the or since that's the actual structure of the data when it comes in. But the extra sentence helps!

@CodyCBakerPhD
Copy link
Contributor Author

@garrettmflynn OK, back to the old message plus an extra response on how to resolve

Copy link
Member

@garrettmflynn garrettmflynn left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@CodyCBakerPhD CodyCBakerPhD merged commit 09cc08f into dev Oct 26, 2023
21 of 22 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the swap_language branch October 26, 2023 22:56
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