Support '*' wildcards in --actions selectors as documented (#2224) - #2234
Support '*' wildcards in --actions selectors as documented (#2224)#2234ihistand wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
/gcbrun |
|
PTAL at following error: |
| // escaped); each '*' matches any run of characters, so "mrd*" -> /^mrd.*$/ and | ||
| // "*features*" -> /^.*features.*$/. | ||
| function globToRegExp(pattern: string): RegExp { | ||
| const escaped = pattern.replace(/[.*+?^${}()|[\]\\]/g, "\\$&").replace(/\\\*/g, ".*"); |
There was a problem hiding this comment.
Can you simplify this logic?
There was a problem hiding this comment.
Agreed, that was doing too much at once — escaping every metacharacter including *, then
un-escaping \* back into .*. You had to read both replaces together to see what it did.
Rewritten to split on the wildcards, escape the literal parts, and rejoin:
function globToRegExp(pattern: string): RegExp {
const escapeLiteral = (literal: string) => literal.replace(/[.+?^${}()|[\]\\]/g, "\\$&");
return new RegExp(`^${pattern.split("*").map(escapeLiteral).join(".*")}$`);
}Behaviour is unchanged — the existing matchPatterns tests pass, and I diffed the two
implementations exhaustively over patterns covering every regex metacharacter plus ** and a**b.
There was a problem hiding this comment.
(note that this change isn't pushed yet, awaiting response)
| // fully-qualified action name; otherwise it matches against the unqualified | ||
| // name (last segment), mirroring the exact-match branches below. Wildcards | ||
| // are expected to select many actions, so no ambiguity error applies here. | ||
| const regExp = globToRegExp(pattern); |
There was a problem hiding this comment.
Shouldn't you first to split by components and then apply regexes?
There was a problem hiding this comment.
Happy to switch if that's the semantics you want, but it isn't behaviour-neutral, so I'd rather
confirm the intent than guess.I implemented component-wise matching (split pattern and value on
., require the same number of
parts, apply one regex per part) and diffed it against the current implementation. One case
changes: whether*may cross a dot.The values here are
targetAsReadableStringoutput —project.dataset.name, ordataset.name
whendefaultProjectisn't set. On the usual three-part name:
pattern current component-wise *,orders,orders*same same *.dataset.orders,*.dataset.*same same *.ordersevery …ordersactionnothing Component-wise is the conventional glob rule (
*stops at the separator, like shell*and/),
and I'm not against it. The one thing that gives me pause is that--actions "*.orders"reads like
"the orders table in whichever dataset", and component-wise it selects nothing on a three-part
name — the user has to know to write*.*.orders. Since wildcards deliberately don't raise the
no-match/ambiguity error, that failure is silent. Letting*span dots avoids it, at the cost of
being less strict.Which would you prefer? If component-wise, I'll push it with tests pinning the part-count
behaviour explicitly.(Probably out of scope here, but worth separating out:
dataset.*matches nothing under either
scheme on a three-part name, because a pattern containing.is matched against the fully-qualified
name. Exactdataset.ordersbehaves the same way today, so the wildcard branch is at least
consistent with the existing rule — happy to look at that separately if it's worth changing.)
…co#2224) matchPatterns did plain string equality only, so the '*' wildcards the --actions help text (run + compile) advertises never matched anything — `run --actions "*"` / "mrd*" reported "No actions to run." Compile wildcard patterns to an anchored RegExp: non-'*' characters match literally (regex metacharacters escaped, so '.' stays a literal dot), each '*' becomes '.*'. A pattern containing '.' matches the fully-qualified action name, otherwise the unqualified last segment — mirroring the existing exact-match branches. Wildcards bypass the ambiguous-name error since matching many actions is the intent; exact selection is unchanged. Adds a matchPatterns test suite (previously untested). Fixes dataform-co#2224
18e4fb3 to
7df93b7
Compare
Fixes #2224 — following up on #2224 (comment) ("Feel free to send a PR :)").
Problem
The
--actionshelp text forrunandcompilesays patterns "can include '*' wildcards", butmatchPatternsincore/utils.tsdoes plain string equality only, so no wildcard pattern ever matches —dataform run --actions "*"reportsNo actions to run.even when the compiled graph has actions.Fix
Wildcard patterns are compiled to an anchored
RegExp:*character matches literally (regex metacharacters are escaped, so.stays a literal dot);*matches any run of characters (mrd*→/^mrd.*$/,*features*→/^.*features.*$/).Scoping mirrors the existing exact-match branches: a pattern containing
.matches against the fully-qualified action name; otherwise it matches against the unqualified last segment. Wildcard matches bypass the ambiguous-name error since selecting many actions is the intent. Exact (non-wildcard) selection behavior is completely unchanged.Tests
Adds a
matchPatternssuite tocore/utils_test.ts(the function was previously untested) covering: exact unqualified/qualified selection, the ambiguity error, bare*, prefix and substring wildcards, qualified wildcards (schema.*), no-match returning empty, and the literal-dot escaping edge case.Understood that GCP Dataform's hosted actions filter doesn't use this implementation — this change only brings the open-source CLI in line with its own documented behavior.
🤖 Generated with Claude Code
https://claude.ai/code/session_0171FwKo8gRQQ35VoYDtSHNU