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

fix version bug when incrementing versions on a prereleased model #91

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

dave-connors-3
Copy link
Collaborator

Closes #90

Rather than relying on latest_version from the model yml to determine the filepath of the next version, we should use the defined versions in yml, since latest_version may not equal the last version for a model defined.

@rijnhardtkotze
Copy link

rijnhardtkotze commented Jul 7, 2023

Hey @dave-connors-3

Thanks for your work on this! This is very cool.

AFAIK from the schema of the ModelNode, the version can be a string as well. We are assuming that it is a number in this PR.

Will that always be the case?

@dave-connors-3 dave-connors-3 changed the title update filename logic using yml definitions, update tests fix version bug when incrementing versions on a prereleased model Jul 7, 2023
@dave-connors-3
Copy link
Collaborator Author

@rijnhardtkotze fantastic question! i was under the impression that these will always be integer values, but you're 100% correct that string values are allowed here -- let me ask around internally about why we're allowing strings. If there's good reason, I can adjust this behavior to handle those cases!

@rijnhardtkotze
Copy link

It also seems weird to me that strings are allowed in versions.

I can imagine it being quite weird when you define a version foo on model bar.

The end result would look out of place with the floating v in there. bar_vfoo.

Perhaps this is a discussion for dbt-core?

@graciegoheen
Copy link
Collaborator

@rijnhardtkotze I believe that core is not strict about requiring numeric versions, because users may be interested in using non-integer versions for their models (such as semantic versioning e.g. 1.0.0).

That being said, I think it makes sense for this package to use integer-only versions - in an attempt to enforce "best practices".

I'm going to go ahead an approve & merge this PR, but if this becomes an issue in the future we can re-visit :)

@graciegoheen graciegoheen merged commit 474617d into main Jul 10, 2023
1 check passed
@nicholasyager nicholasyager deleted the fix/duplicate-versions branch July 25, 2023 20:35
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.

duplicate versions defined after using --prerelease flag for version operation
3 participants