-
Notifications
You must be signed in to change notification settings - Fork 1
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 tuple as input type in parse_excel.infer_metadata()
#213
Conversation
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.
Is there are case where units
is actually a list
? Or is it always (now) a tuple
? If so, the list
type should simply be changed to tuple
without the Union
.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #213 +/- ##
=======================================
Coverage 88.67% 88.67%
=======================================
Files 15 15
Lines 459 459
=======================================
Hits 407 407
Misses 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
No, it is only called using a |
If you want to make it even more accurate, you can pass in the types that will be contained in the tuple, for example if it's two strings: |
@@ -153,7 +153,7 @@ def split_column_name(column): | |||
return name, unit | |||
|
|||
|
|||
def infer_metadata(rec: np.recarray, units: list) -> dlite.Instance: | |||
def infer_metadata(rec: np.recarray, units: tuple) -> dlite.Instance: |
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.
Since any sequence will work, I will suggest to state that explicitly:
def infer_metadata(rec: np.recarray, units: tuple) -> dlite.Instance: | |
def infer_metadata(rec: np.recarray, units: "Sequence") -> dlite.Instance: |
This requires that we load from typing import Sequence
.
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.
I agree, but this does not work together with @CasperWA's suggestion to also specify an unknown number of strings. (That results in TypeError: Parameters to generic types must be types. Got Ellipsis.
). I will hence go for tuple[str, ...]
.
Thank you! I implemented the |
Description:
parse_excel.infer_metadata()
expects its input parameterunits
as alist
, but we want to send atuple
fromget()
. Suggested solution is to add handling oftuple
.Closes #212.
Type of change:
Checklist for the reviewer:
This checklist should be used as a help for the reviewer.