-
Notifications
You must be signed in to change notification settings - Fork 7
Add id
field to v2.ProblemConfig
#442
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
Conversation
* Update schema * v2.Problem.id * v2.ProblemConfig.id Related to PEtab-dev/PEtab#646. Closes PEtab-dev#441.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #442 +/- ##
==========================================
- Coverage 74.82% 74.80% -0.02%
==========================================
Files 62 62
Lines 6792 6803 +11
Branches 1203 1205 +2
==========================================
+ Hits 5082 5089 +7
- Misses 1255 1258 +3
- Partials 455 456 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
👍
petab/v2/core.py
Outdated
@property | ||
def id(self) -> str: | ||
"""The ID of the PEtab problem if set, ``None`` otherwise.""" | ||
return self.config.id if self.config else None |
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.
@property | |
def id(self) -> str: | |
"""The ID of the PEtab problem if set, ``None`` otherwise.""" | |
return self.config.id if self.config else None | |
@property | |
def id(self) -> str | None: | |
"""The ID of the PEtab problem if set, ``None`` otherwise.""" | |
return self.config.id if self.config else None |
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.
Ah, yeah, I was undecided whether we should go for None
or ''
if no ID is set. I think always having a string could be more convenient. Any preference?
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.
Hm, I think the Pythonic way would be to use None
, because it's the absence of an ID. Also the "no ID" case is a special case that will always need to be handled, e.g. to avoid writing a file name .yaml
to disc. Maybe using None
then makes it easier to detect errors since the special case is more explicit. No strong opinion 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.
Memo to self: The yaml schema may need updating. If we distinguish empty strings and null/None, the former should raise a validation error whereas the latter should not.
Related to PEtab-dev/PEtab#646.
Closes #441.