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

Validate that the metadata is always a dict #466

Merged
merged 5 commits into from
Oct 20, 2023
Merged

Conversation

R-Palazzo
Copy link
Contributor

@R-Palazzo R-Palazzo commented Oct 12, 2023

Resolve #428

@R-Palazzo R-Palazzo requested a review from a team as a code owner October 12, 2023 14:42
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (bc92a5e) 76.84% compared to head (cc6a105) 76.87%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
+ Coverage   76.84%   76.87%   +0.02%     
==========================================
  Files          93       93              
  Lines        3395     3395              
==========================================
+ Hits         2609     2610       +1     
+ Misses        786      785       -1     
Files Coverage Δ
sdmetrics/reports/base_report.py 100.00% <100.00%> (+1.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@npatki
Copy link
Contributor

npatki commented Oct 12, 2023

@R-Palazzo we should not be accepting the actual metadata object. We should only accept dictionaries.

I would avoid doing any conversions in SDMetrics and stick to the original issue.

"""If the metadta is not a dict, try to convert it."""
if not isinstance(metadata, dict):
try:
metadata = metadata.to_dict()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something we actually want to support? Other objects might implement a to_dict method which might lead to weird issues down the road (i.e. I'm pretty sure you can call to_dict on a pandas dataframe). Maybe @npatki has thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

@frances-h agreed. I think we should stick to the issue's description, which states that we should only accept dictionaries. We do not intend to convert to a dictionary on the user's behalf.

Copy link
Contributor

@amontanez24 amontanez24 left a comment

Choose a reason for hiding this comment

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

LGTM!

@R-Palazzo R-Palazzo merged commit 99cb1e4 into main Oct 20, 2023
45 checks passed
@R-Palazzo R-Palazzo deleted the issue-428-metadata-dict branch October 20, 2023 15:20
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.

Validate that the metadata is always a dict
5 participants