Improve understanding of revisions vs filters [#706] #711
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,12 @@ | ||
| import re | ||
| import warnings | ||
|
|
||
| import click | ||
| import pygit2 | ||
|
|
||
| from kart.cli_util import KartCommand, RemovalInKart012Warning | ||
| from kart.exceptions import NotFound | ||
|
|
||
|
|
||
| class PreserveDoubleDash(KartCommand): | ||
| """ | ||
|
|
@@ -39,29 +39,15 @@ def parse_args(self, ctx, args): | |
| return super(PreserveDoubleDash, self).parse_args(ctx, args) | ||
|
|
||
|
|
||
| def _separate_options(args, allow_options): | ||
| options = [] | ||
| others = [] | ||
| for arg in args: | ||
| if not arg.startswith("-"): | ||
| others.append(arg) | ||
| elif not allow_options: | ||
| raise click.UsageError(f"No such option: {arg}") | ||
| else: | ||
| # It's not explicitly stated by https://git-scm.com/docs/git-check-ref-format | ||
| # but this isn't a valid commit-ish. | ||
| # $ git branch -c -- -x | ||
|
|
@@ -75,29 +61,89 @@ def get_arg_type(repo, arg, allow_options=True, allow_commits=True, allow_filter | |
| f"This will be removed in Kart 0.12! Please comment on {issue_link} if you need to use this option.", | ||
| RemovalInKart012Warning, | ||
| ) | ||
| options.append(arg) | ||
| return options, others | ||
|
|
||
|
|
||
| def _append_kwargs_to_options(options, kwargs, allow_options): | ||
| if not kwargs: | ||
| return | ||
| assert allow_options | ||
|
|
||
| for option_name, option_val in kwargs.items(): | ||
| option_name = option_name.replace("_", "-", 1) | ||
| t = type(option_val) | ||
| if t == int or t == str: | ||
| options.append(f"--{option_name}={option_val}") | ||
| elif t == tuple: | ||
| options.extend([f"--{option_name}={o}" for o in option_val]), | ||
| elif t == bool and option_val: | ||
| options.append(f"--{option_name}") | ||
|
|
||
|
|
||
| HEAD_PATTERN = re.compile(r"^HEAD\b", re.IGNORECASE) | ||
craigds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| HINT = "Use '--' to separate paths from revisions, like this:\n'kart <command> [<revision>...] -- [<filter>...]'" | ||
| SILENCING_HINT = "To silence this warning, use '--' to separate paths from revisions, like this:\n'kart <command> [<revision>...] -- [<filter>...]'\n" | ||
|
|
||
|
|
||
| def _is_revision(repo, arg, warning_set): | ||
| # These things *could* be a path, but in that case the user should add a `--` before this arg to | ||
| # disambiguate, and they haven't done that here. | ||
| if arg == "[EMPTY]" or ".." in arg or HEAD_PATTERN.match(arg) or arg.endswith("^?"): | ||
craigds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return True | ||
|
|
||
| filter_path = arg.split(":", maxsplit=1)[0] | ||
| head_tree = repo.head_tree | ||
| is_useful_filter_at_head = head_tree and filter_path in head_tree | ||
|
There was a problem hiding this comment. how's There was a problem hiding this comment. I only pass the first part of a filter to it - everything before the ":" - which is a path with no particular encoding. Everything after that is ignored. So if you have an arg like "points:feature:25" and a dataset called "points" and no revision called "points", we're happy to assume your arg is a filter - even without encoding the feature PK to see if it exists, it's enough that the dataset exists. |
||
|
|
||
| if ":" in arg and not is_useful_filter_at_head: | ||
| click.echo( | ||
| f"Assuming '{arg}' is a filter argument (that doesn't match anything at HEAD)", | ||
| err=True, | ||
| ) | ||
| warning_set.add(SILENCING_HINT) | ||
| return False | ||
|
|
||
| try: | ||
| repo.resolve_refish(arg) | ||
| is_revision = True | ||
| except (KeyError, ValueError, pygit2.InvalidSpecError): | ||
| is_revision = False | ||
|
|
||
| if is_revision and not is_useful_filter_at_head: | ||
| return True | ||
| elif is_useful_filter_at_head and not is_revision: | ||
| return False | ||
| elif is_revision and is_useful_filter_at_head: | ||
| raise click.UsageError( | ||
| f"Ambiguous argument '{arg}' - could be either a revision or a filter\n{HINT}" | ||
| ) | ||
| else: | ||
| raise NotFound( | ||
| f"Ambiguous argument '{arg}' - doesn't appear to be either a revision or a filter\n{HINT}" | ||
| ) | ||
|
|
||
|
|
||
| def _disambiguate_revisions_and_filters(repo, args): | ||
| revisions = [] | ||
| filters = [] | ||
| warning_set = set() | ||
|
There was a problem hiding this comment. not sure this needs to be a set; it looks like you're only adding SILENCING_HINT, so there should be other ways to dedupe that. Making it a set means the output will be nondeterministically ordered if you ever add a second thing to the set. Perhaps we should configure the warnings module? It will de-dupe warnings automatically if configured correctly There was a problem hiding this comment. ... the warnings module looks great for deprecating things in modules that will be imported by third parties, etc etc. But I am just trying to print something once: I don't need auto line numbers + module names, or configuration options, and I especially don't want it to remember which warnings it has already printed for any longer than this function call (as could happen with our testing framework / kart helper). It's not beautiful how it is but I've just spent 10 minutes thinking about this and I think it's not really worth spending any longer on it - there's actually nothing measurably wrong with it right now There was a problem hiding this comment.
yep, that could use some work. Looks like there's no simple way to reset the warning registry globally: https://bugs.python.org/issue21724 - which we'd need to make this work well for kart helper
AFAIK pytest resets the warnings state between each test, by default. Maybe it uses the hacky workaround in that python ticket 🤷 . It also has really good support for the warnings module if you want to check for warnings in your tests: https://docs.pytest.org/en/latest/how-to/capture-warnings.html So in that case, I guess leave this as it is 👍 |
||
| for i, arg in enumerate(args): | ||
| if _is_revision(repo, arg, warning_set): | ||
| if filters: | ||
| raise click.UsageError( | ||
| f"Filter argument '{filters[0]}' should go after revision argument '{arg}'\n{HINT}" | ||
| ) | ||
| revisions.append(arg) | ||
| else: | ||
| filters.append(arg) | ||
| for warning in warning_set: | ||
| click.echo(warning, err=True) | ||
| return revisions, filters | ||
|
|
||
|
|
||
| def parse_revisions_and_filters( | ||
| repo, | ||
| args, | ||
| kwargs=None, | ||
|
|
@@ -108,6 +154,9 @@ def parse_commits_and_filters( | |
| Returns a three-tuple: (options, commits/refs/ranges, filters) | ||
| """ | ||
|
|
||
| # If kwargs are passed, allow_options must be set. | ||
| assert allow_options or not kwargs | ||
|
|
||
| # As soon as we encounter a filter, we assume all remaining args are also filters. | ||
| # i.e. the filters must be given *last*. | ||
| # If it's ambiguous whether something is a filter or not, we assume it's a commit-ish. | ||
|
|
@@ -118,42 +167,11 @@ def parse_commits_and_filters( | |
| dash_index = args.index("--") | ||
| filters = list(args[dash_index + 1 :]) | ||
| args = args[:dash_index] | ||
| options, revisions = _separate_options(args, allow_options) | ||
| _append_kwargs_to_options(options, kwargs, allow_options) | ||
| return options, revisions, filters | ||
| else: | ||
| options, args = _separate_options(args, allow_options) | ||
| _append_kwargs_to_options(options, kwargs, allow_options) | ||
| revisions, filters = _disambiguate_revisions_and_filters(repo, args) | ||
| return options, revisions, filters | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use isinstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good call but I find it amusing how you recommend style changes to your own code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. not sure what i was thinking when i wrote it, that code doesn't seem like something I would write normally. Maybe I wrote it before I had a ☕