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

chore: allow dataclasses >= 0.6, < 0.9 #3151

Conversation

bastienboutonnet
Copy link
Contributor

@bastienboutonnet bastienboutonnet commented Mar 8, 2021

resolves #3150

Description

As asked in #3150 dataclasses could be allowed to vary more to make dbt cohabit with other projects which rely on version of dataclasses >0.6.

@jtcohen6 thanks for the pointers in the ticket

A long time ago when I contributed to dbt I remember the CI doesn't run on external PRs is that still the case? If so, how should we go about checking that this change doesn't break as you instructed in #3150 ?

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
    • well, i've been able to add dbt as a dependency on dbt-sugar via poetry successfully because the resolver isn't upset anymore ✨
  • 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 next" section.

@cla-bot cla-bot bot added the cla:yes label Mar 8, 2021
@bastienboutonnet bastienboutonnet marked this pull request as ready for review March 8, 2021 20:15
@gshank
Copy link
Contributor

gshank commented Mar 8, 2021

This pull request isn't mergeable because it points to your private versions of hologram. I guess you just want to see if the CircleCI tests pass?

@bastienboutonnet
Copy link
Contributor Author

bastienboutonnet commented Mar 8, 2021

@gshank yes that's what @jtcohen6 advised to do. So it will go like this I guess:

  1. get the hologram PR merged: chore: allow dataclasses to up up to 0.9 excluding hologram#42
  2. if we're good here (which it looks it is) we can point to the newer version of hologram (I think I'll have to wait for a green light to know if a new version tag has been generated and the release is available on PyPI
  3. check all works fine still and then you folks should be able to merge it.

@bastienboutonnet
Copy link
Contributor Author

@gshank ah I see you merged the hologram PR. Let me know when there's a new version that I can replace the pointer to fork with

@gshank
Copy link
Contributor

gshank commented Mar 8, 2021

There's a new version, 0.0.14

@bastienboutonnet
Copy link
Contributor Author

Damn that's blazing fast @gshank I'll update in a wee bit.

@bastienboutonnet
Copy link
Contributor Author

bastienboutonnet commented Mar 8, 2021

@gshank alright change done.

Btw, I had a question in the PR description around this checklist item:

I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.
not sure if this is worthy of a changelog update but if so let me know and I can add a little thing

Let me know what you think and I can change accordingly.

Also, which release of dbt can we expect this change to be rolled into and when would that be (roughly)?

core/setup.py Outdated
@@ -69,8 +69,8 @@ def read(fname):
'isodate>=0.6,<0.7',
'json-rpc>=1.12,<2',
'werkzeug>=0.15,<2.0',
'dataclasses==0.6;python_version<"3.7"',
'hologram==0.0.13',
'dataclasses==0.7;python_version<"3.7"',
Copy link
Contributor Author

@bastienboutonnet bastienboutonnet Mar 8, 2021

Choose a reason for hiding this comment

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

also actually I'm not sure if we should pin strictly to 0.7 here or let it be like the hologram depenency which is >=0.6,<0.9

I gravitate towards the latter otherwise we're not really solving the issue of having too strict dependencies, what do you think @gshank ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with having this match the hologram dependency exactly: 'dataclasses>=0.6,<0.9;python_version<"3.7"' feels right.

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Nice quick work @bastienboutonnet! Thanks for the assist @gshank.

I definitely think this is worthy of a Changelog update + adding yourself to the list of Contributors :)

core/setup.py Outdated
@@ -69,8 +69,8 @@ def read(fname):
'isodate>=0.6,<0.7',
'json-rpc>=1.12,<2',
'werkzeug>=0.15,<2.0',
'dataclasses==0.6;python_version<"3.7"',
'hologram==0.0.13',
'dataclasses==0.7;python_version<"3.7"',
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with having this match the hologram dependency exactly: 'dataclasses>=0.6,<0.9;python_version<"3.7"' feels right.

@jtcohen6 jtcohen6 mentioned this pull request Mar 9, 2021
4 tasks
@jtcohen6 jtcohen6 added the dependencies Changes to the version of dbt dependencies label Mar 9, 2021
@bastienboutonnet
Copy link
Contributor Author

Thanks @jtcohen6 I added an entry in the changelog. Not sure if it will be going in 0.20.0 so I'm happy to move it. I couldn't find a dbt next section. Let me know if you think it fits better somewhere else.

@bastienboutonnet bastienboutonnet force-pushed the chore/bump_hologram_and_dataclasses branch from 9a4ed17 to 34174ab Compare March 9, 2021 14:56
Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

Thanks for this @bastienboutonnet!

@jtcohen6 jtcohen6 merged commit 24e4b75 into dbt-labs:develop Mar 9, 2021
@bastienboutonnet
Copy link
Contributor Author

Pleasure as always @jtcohen6 and thanks @gshank for your help on this yesterday too!

@bastienboutonnet bastienboutonnet deleted the chore/bump_hologram_and_dataclasses branch March 9, 2021 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes dependencies Changes to the version of dbt dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could the dataclasses requirement be relaxed?
3 participants