Improve understanding of revisions vs filters [#706] #711
Conversation
Don't assume things are filters just because they seem not to be revisions - they may be a revision with a typo in them. Instead, check to make sure filters are present at HEAD, and fail with an error message if we can't decide if something is a revision or a filter. Also fixes up some naming.
kart/parse_args.py
Outdated
| 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}") |
There was a problem hiding this comment.
👍 good call but I find it amusing how you recommend style changes to your own code
There was a problem hiding this comment.
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 ☕
kart/parse_args.py
Outdated
| 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.
kart helper
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
as could happen with our testing framework
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 👍
| def parse_commits_and_filters( | ||
| 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 filter_path in head_tree work? It looks like filter_path is something like ["<datasetname>", "<pk_value>"] and you're passing it to a pygit2.Tree.__contains__, which doesn't know about our path encodings. Or am I missing something?
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.
Don't assume things are filters just because they seem not to be revisions - they may be a revision with a typo in them. Instead, check to make sure filters are present at HEAD, and fail with an error message if we can't decide if something is a revision or a filter.
Also fixes up some naming.
Fixes #706
Changelog doesn't need updating: already refers to this issue for the next release.