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

Be nicer about possible misconfigurations #1004

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

thejcannon
Copy link
Contributor

Fixes #1003 by turning situations where the user might get an AssertionError and turn them into descriptive ValueErrors.

Additionally, tests have been added to test the behavior.

The situations fixed are (with the real-world facepalm situations I ended up in):

  • The entrypoint for version_scheme and local_scheme don't exist
    • 🤦 I accidentally used the function name once, instead of the entrypoint name
  • The entrypoint(s) for version_scheme and local_scheme don't return anything
    • 🤦 I forgot to add a return to my custom scheme
  • The tag_regex is bad, and was loaded from the config
    • 🤦 This one should be self-explanatory
  • The tag_regex matched, but the version was blank
    • 🤦 My regex was borked

Copy link
Contributor

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

i like the idea behind how the details change, but i aslo beleive a larger change is necessary to do the path/version scheme handling in a better way

due to personal time constraints it may take a be a while to propperly reiterate this

thank you for getting the ball rolling and preparing this as a initial vesion

warnings.warn(
"Expected tag_regex to contain a single match group or a group named"
" 'version' to identify the version part of any tag."
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

this one is possibly a breaking change im not sure we can integrate it off hand

@@ -105,6 +105,9 @@ class Configuration:

parent: _t.PathT | None = None

def __post_init__(self) -> None:
self.tag_regex = _check_tag_regex(self.tag_regex)
Copy link
Contributor

Choose a reason for hiding this comment

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

taking this here without leaving it in place creates potental trouble for valies valid in config but not in the config objects

@@ -139,13 +142,11 @@ def from_data(
given configuration data
create a config instance after validating tag regex/version class
"""
tag_regex = _check_tag_regex(data.pop("tag_regex", None))
Copy link
Contributor

Choose a reason for hiding this comment

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

this check needs to say even if the validation gets more struct

@RonnyPfannschmidt
Copy link
Contributor

i intend to merge this after hte next minor release as its a "breaking change" that should go in nonetheless

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.

Be a smidge more error tolerant?
2 participants