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

Use new agate_helper.Number type in tests for table creation #76

Merged
merged 3 commits into from
Jan 5, 2022

Conversation

anthu
Copy link
Contributor

@anthu anthu commented Dec 22, 2021

Description

The PR dbt-labs/dbt-core#4512 fixed bool coercion. As part of the change the TypeTester changed the available Number Type from agate.data_types.number.Number to agate_helper.Number.

While this is well fine in terms of inheritance the tests are failing now, see:
#74 https://github.com/dbt-labs/dbt-snowflake/runs/4610851479?check_suite_focus=true
#75 https://github.com/dbt-labs/dbt-snowflake/runs/4606768468?check_suite_focus=true

This PR fixes the tests to use the new Number type

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt-snowflake next" section.

@McKnight-42
Copy link
Contributor

@anthu Thank you so much for working taking the initiative on this fix, we've actually been making fixes for this in other repos as well noted https://github.com/dbt-labs/dbt-bigquery/pull/93/files and dbt-labs/dbt-redshift#58 would love to keep these implementations uniform for future maintenance.

@anthu
Copy link
Contributor Author

anthu commented Jan 4, 2022

Hey @McKnight-42,

Yeah, I saw your work in the mentioned PRs earlier today and I like your approach!
Shall I adapt my implementation or can you simply cherrypick it from the other repos?

@McKnight-42
Copy link
Contributor

@anthu if you could adapt yours to the way the others are i can review as soon as tomorrow and we can merge then, i tested it locally on redshift and know it passes the tests as well so should have no issues there. don't want to lose this great collaborative record and keeping you as the main contributor for this as you took the initiative seems right.

@McKnight-42 McKnight-42 merged commit fbf2b80 into dbt-labs:main Jan 5, 2022
@anthu anthu deleted the fix/agate-number-tests branch January 5, 2022 19:22
leahwicz pushed a commit that referenced this pull request Apr 1, 2022
* Use new agate_helper.Number type for table creation

* Revert "Use new agate_helper.Number type for table creation"

This reverts commit 4d72ac7.

* Update test of Number type check from Boolean to 0,1
leahwicz pushed a commit that referenced this pull request Apr 1, 2022
* Use new agate_helper.Number type for table creation

* Revert "Use new agate_helper.Number type for table creation"

This reverts commit 4d72ac7.

* Update test of Number type check from Boolean to 0,1
leahwicz added a commit that referenced this pull request Apr 1, 2022
* Remove unneeded Core download in CI (#127)

* Update dev_requirements for 1.0.latest branch

* Use new agate_helper.Number type in tests for table creation (#76)

* Use new agate_helper.Number type for table creation

* Revert "Use new agate_helper.Number type for table creation"

This reverts commit 4d72ac7.

* Update test of Number type check from Boolean to 0,1

Co-authored-by: Anton Huck <anthu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants