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

Update tests to use Metadata #2178

Merged
merged 12 commits into from
Aug 14, 2024
Merged

Conversation

lajohn4747
Copy link
Contributor

resolves #2151
CU-86b1efup0

Moves tests to use Metadata.
This does two things:

  • Confirms the Metadata interface is working fine with our library
  • Helps users/developers to transition over to Metadata. Keeps conformity within the library.

@sdv-team
Copy link
Contributor

sdv-team commented Aug 9, 2024

sdv/single_table/base.py Outdated Show resolved Hide resolved
sdv/metadata/metadata.py Outdated Show resolved Hide resolved
if isinstance(metadata, Metadata):
self.table_name = metadata._get_single_table_name()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this scenario. If multiple tables are in the metadata, then we error in the single table case so we don't need the warning. Maybe we should also just error if the user doesn't pass a table name in instead of replacing with a default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This scenario is if there is a single table, we want to make sure get_metadata returns the single table object with the correct name attached to the table (same one that is passed in) if there isn't a name (e.g. legacy SingleTableMetadata passed in) we can use the default name.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a legacy metadata then it won't enter this branch of the if right? And if it's the legacy metadata why do we need a name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, legacy metadata will just have the default name so it's not the best example.

When we call get_metadata that comes out with the class Metadata. That table should match whatever was put into the class so the user can refer to the table using the same table name.

Base automatically changed from issue_2131_metadata_for_demos to feature/metadata August 13, 2024 14:07
@lajohn4747 lajohn4747 merged commit 6a2bdc3 into feature/metadata Aug 14, 2024
39 checks passed
@lajohn4747 lajohn4747 deleted the issue_2151_update_tests branch August 14, 2024 15:36
R-Palazzo pushed a commit that referenced this pull request Sep 19, 2024
R-Palazzo pushed a commit that referenced this pull request Sep 24, 2024
R-Palazzo pushed a commit that referenced this pull request Sep 26, 2024
R-Palazzo pushed a commit that referenced this pull request Sep 26, 2024
amontanez24 pushed a commit that referenced this pull request Sep 27, 2024
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.

4 participants