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

Remove APIObject (#1762) #1780

Merged

Conversation

beckjake
Copy link
Contributor

Convert Relation types to use hologram.JsonSchemaMixin instead of APIObject, and remove it.

Fixes #1762

  • Remove APIObject
  • Split relations and columns into separate files (drew was right, I should have done this initially)
  • split context.common into base + common
    • base is all that's required for the config renderer
  • Fixed a lot of annoying mypy things
  • factored _extra behavior out of node configs
  • Moved Credentials into connection contracts since that's what they really are
  • Removed model_name/table_name -> consolidated to identifier
    • I hope I did not break seeds, which claimed to care about render(False) but look like they're fine
  • Unified shared 'external' relation type with bigquery's own
  • added hack workarounds for some import cycles with plugin registration and config p
    arsing
  • Assorted backwards compatibility fixes around Relation and macros

@cla-bot cla-bot bot added the cla:yes label Sep 25, 2019
@beckjake beckjake force-pushed the refactor/relations-as-jsonschemamixins branch from 5f743cb to 3e95e43 Compare September 26, 2019 18:29
@beckjake beckjake requested a review from drewbanin September 26, 2019 18:56
@beckjake beckjake force-pushed the refactor/relations-as-jsonschemamixins branch 2 times, most recently from 406117f to da86314 Compare September 27, 2019 14:45
Fix a lot of mypy things, add a number of adapter-ish modules to it
Split relations and columns into separate files
split context.common into base + common
 - base is all that's required for the config renderer
Move Credentials into connection contracts since that's what they really are
Removed model_name/table_name -> consolidated to identifier
 - I hope I did not break seeds, which claimed to care about render(False)
Unify shared 'external' relation type with bigquery's own
hack workarounds for some import cycles with plugin registration and config p
arsing
Assorted backwards compatibility fixes around types, deep_merge vs shallow merge
Remove APIObject
@beckjake beckjake force-pushed the refactor/relations-as-jsonschemamixins branch from da86314 to eb9bfcd Compare September 27, 2019 15:01
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

There's a lot going on here! It looks to me like table_name can no longer be provided when creating a Relation object. That's fine, I'm glad to move on from that, but we should note it as a breaking change in the changelog!

When giving this a spin locally, I ran into an error indicating that typing_extensions wasn't installed. I was able to work around it by installing this package. I'm using python 3.7.4 -- is that expected?

LGTM pending the questions above

@beckjake
Copy link
Contributor Author

beckjake commented Oct 2, 2019

When giving this a spin locally, I ran into an error indicating that typing_extensions wasn't installed. I was able to work around it by installing this package. I'm using python 3.7.4 -- is that expected?

I guess that should be expected, though it's really weird that the tests passed!

add typing extensions module to setup.py
Update changelog
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

ship it

@beckjake beckjake merged commit 31e085b into dev/louisa-may-alcott Oct 2, 2019
@beckjake beckjake deleted the refactor/relations-as-jsonschemamixins branch October 2, 2019 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove APIObject
2 participants