Add filter argument to update-drafts script #1679
Conversation
ddbeck
added
the
tools and infrastructure
scripts/update-drafts.ts
Outdated
| @@ -1,3 +1,7 @@ | |||
| // Updates files in features/draft/spec/* with BCD keys mentioned in specs | |||
| // `npm run update-drafts` updates all | |||
| // `npm run update-drafts -- [key]` only updates features with a `shortname` that includes `key`. | |||
There was a problem hiding this comment.
Do I understand correctly that this matches a key, not a file name? I think that's surprising and slightly inconvenient (e.g., it'd be nice to be able to use shell globbing or xargs to run this).
What do you think of using file names as arguments, then getting the key from the filename (kinda like dist does for yml and dist files)?
Or if you like the idea of providing a partial string (like your ext example), maybe we do need yargs, so we can have something like --by-key and --by-path options.
There was a problem hiding this comment.
Yes, you're correct that it takes a key, so paint-order would match but paint-order.yml or features/draft/spec/css-transforms-2.yml would not.
I agree that file paths would make sense to support, I can update to do that (and will go ahead and add yargs).
| const argv = yargs(process.argv.slice(2)) | ||
| .scriptName("update-drafts") | ||
| .usage("$0", "Update draft features with BCD keys mentioned in specs.") | ||
| .option("keys", { | ||
| type: "array", | ||
| describe: "Keys to match", | ||
| }) | ||
| .option("paths", { | ||
| type: "array", | ||
| describe: "Draft feature files to update", | ||
| }).argv; | ||
|
|
There was a problem hiding this comment.
So CLIs are always more complicated than I imagine. 😅
This behaves very strangely if both options are set. I think it's applying both filters, such that nothing gets to match? I'm not sure.
I think the right thing to do here is to make sure the options don't layer on top of each other (or set some kind of flag like --matches=id, --matches=path, and then all positional arguments must be one type). But this is starting to get complicated and holding up useful work.
How about this: would you like to revert the changes after my review, add a # TODO: linking back here and one of us can take that up in a follow up PR?
There was a problem hiding this comment.
It was a simple change to make it an AND- for instance, npx tsx scripts/update-drafts.ts --keys css --paths features/draft/spec/ba* will update all CSS specs AND badging.yml and battery-status.yml.
If this doesn't seem correct, I'll revert and TODO.
There was a problem hiding this comment.
Nearly there! See my comment below.
scripts/update-drafts.ts
Outdated
| for (const spec of webSpecs) { | ||
|
|
||
| let selectedSpecs = webSpecs; | ||
| let selectedKeys = filterKeys; |
There was a problem hiding this comment.
OK this almost works now! If you don't supply keys though, this value being undefined fails.
| let selectedKeys = filterKeys; | |
| let selectedKeys = filterKeys ?? []; |
There was a problem hiding this comment.
Good catch! Thanks!
| const argv = yargs(process.argv.slice(2)) | ||
| .scriptName("update-drafts") | ||
| .usage("$0", "Update draft features with BCD keys mentioned in specs.") | ||
| .option("keys", { | ||
| type: "array", | ||
| describe: "Keys to match", | ||
| }) | ||
| .option("paths", { | ||
| type: "array", | ||
| describe: "Draft feature files to update", | ||
| }).argv; | ||
|
|
There was a problem hiding this comment.
Nearly there! See my comment below.
While working through a set of features, it's helpful re-run
update-draftsto see progress, but it makes things a bit messy to update all. This allows you to run:npm run update-drafts -- --keys extto update all drafts with a key that includesextnpm run update-drafts -- --keys ext-blend-minmaxto update a specific draft.npm run update-drafts -- --paths features/draft/spec/css-*to update all drafts based on filename.I opted to not port to yargs, unless there is likelihood for further options.