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

feature/deps-regex-semver #5370

Merged

Conversation

fivetran-joemarkiewicz
Copy link
Contributor

resolves #5201

Description

In this PR I incorporate the suggested regular expression (RegEx) to check a SemVer string for when dbt deps is invoked. Truth be told, when breaking out this expression, there wasn't too much difference from what was included previously. Please let me know if I missed anything.

In addition to the above, I updated the error that is shown when the versioning does not fit within the new regex rules. This error message will be descriptive to show users that the version most likely needs to be modified.

Finally, I added unit testing to ensure all variations of the valid versions shown here are truly valid, and that all variations of the invalid versions shown here are truly invalid. Please note, I ended up creating a new function to simply test if the version triggered the error message upon invoking the from_version_string function. Please let me know if this was not needed and there was just a unit test I was simply missing! This is my first attempt at unit tests so happy to modify where needed! 😄

CLI Output

Based off these changes, if the version provided for a package does not match the regex rules then we should see a clearer error. See below for my validation of the error message
image

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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 opened an issue to add/update docs, or docs changes are not required/relevant for this PR
    ☝️ I don't think this is needed, but please let me know and I can if that is required.
  • I have run changie new to create a changelog entry

@fivetran-joemarkiewicz fivetran-joemarkiewicz requested a review from a team as a code owner June 13, 2022 22:23
@cla-bot cla-bot bot added the cla:yes label Jun 13, 2022
@jtcohen6 jtcohen6 added Team:Language ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering labels Jun 14, 2022
@@ -95,7 +95,7 @@ def from_version_string(cls, version_string):

if not match:
raise dbt.exceptions.SemverException(
'Could not parse version "{}"'.format(version_string)
'"{}" is not a valid semantic version.'.format(version_string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'"{}" is not a valid semantic version.'.format(version_string)
f'"{version_string}" is not a valid semantic version.'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this change in the most recent commit. Thanks!

@@ -427,6 +427,15 @@ def resolve_to_specific_version(requested_range, available_versions):
return max_version_string


def semver_regex_versioning(versions: List[str]) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just for testing? Can we move it into the test file if that's the case?

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 is just for testing the various version types. I moved it into the testing file as that does make a lot more sense 😄

Copy link
Contributor

@leahwicz leahwicz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @fivetran-joemarkiewicz for the contribution 🎉

@leahwicz leahwicz merged commit c42221f into dbt-labs:main Jul 6, 2022
agoblet pushed a commit to BigDataRepublic/dbt-core that referenced this pull request Sep 16, 2022
* feature/deps-regex-semver

* pre-commit fixes

* applying review updates
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ok_to_test ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-589] [Bug] dbt deps does not precisely match the regex for SemVer
4 participants