Andrea Vos (392c6595) at 19 Mar 17:32

disagreed, your MR remains the main one 😉 mine is based on a slightly outdated branch, doesn't have previous discussions, etc. – but if you're onboard with the structure, please incorporate this simpler scaffolding into your branch (merging my branch directly into yours is not recommended, because while i created the structure itself manually, refactoring all the commands into that structure was entirely vibe coded and not reviewed, just a prototype)

yes yes. this PR is not meant to be merged, and definitely not to main directly.

the only point is to discuss the simpler approach to scaffolding. let's keep other discussions to your PR 😉

ooof, i was supposed to create this MR to your branch, not to main 😅 the only relevant changes are in bin

I've proposed a simplification in !759 (merged) ;)

also, just realised that there's also server/miastamaszerujace.ts that's a CLI command (it can be put in the calendar group, next to `bot)

i'm fine with the changes, but the pipeline is failing 😅 i guess more of pnpm caching issues?

generic option works too

totally!

⚠️ This is not production ready, just a PoC for a discussion of !758 (I created a hello-world PoC and told Claude to migrate Ben's commands into this pattern; plus I didn't tackle updates to makefile, readme etc.)

basically, the advantages i'm looking for here are:

• no need for all the complexity in bin/lib/*.ts. complex types requiring docstrings, an extra abstraction of Command… yargs seem to handle strings, numbers and booleans correctly out of the box, and those are the only types we use. zod is really cool, but might be an overkill for this use case, and i'd rather have one dependency fewer 😉
• simplified folder structure: bin/commands/<group>/<subcommand>.ts rather than bin/commands/<group>/main.ts and bin/commands/<group>/subcommands/<subcommand>.ts
• all commands and subcommands defined in one place, bin/index.ts, rather than multiple main.ts files. if we had dozens of commands, a more verbose structure would be justified, but for us right now I'd argue that simpler is better 😉 the resulting bin/index.ts is just 66 lines, yet it lets you see at glance what commands we have and how they're grouped
  • that makes it really convenient to move commands around. i moved cleanup out of utils with just a few actions required
  • speaking of… IMO the generic "utils" group is not helpful, it just makes things unnecessarily verbose. why do i need to type pnpm cli utils calendarBot rather than just pnpm cli calendarBot? i love the subcommands structure that lets us group similar commands, but utils doesn't provide value. instead, we could for example have db migrate and db init-test as one group, maybe admin stats and admin notify as another, and make the rest just top-level commands? of give them each a "group of one", like sentry already is, eg. pnpm cli calendar bot

Andrea Vos (cec549aa) at 19 Mar 14:24

... and 15 more commits

includes some refactoring to make the implementation easier. main thing is that profile.opinions is always an object, that way functions can be more indifferent about what they take (and maybe later enables having a preview in the editor)

includes a simple label header for the opinion inputs, may be sensible to apply it to other forms too

I had something to mention it here, but switched context to the CLI, and forgot 🤦 Oh well, if it comes back to me, I'll add a comment.

Overall, looks great! Personally, I'd prefer backfilling with null rather than 0, but it's not really a hill i'd die on. So if my argument convinces you, then let's do it, but if you wanna merge already, that's a ✅ from me too 😉

I wanna avoid people checking in on their card and wondering "why would PP mark my 'yassss queen' as neutral?! bug report, angry tweet!". instead they could see an unfilled value, which should convey a "this is a new feature, fill it out! (and until you do, we'll keep treating it as neutral)".

if there is no for that value, it will show up as empty at first, but force a choice when clicked.

although, that would force us to think about form validation. if they don't fill out the sentiments for pre-existing opinions, and hit save, should we prevent that and display an error? probably too much bother, but at least we should make sure that this doesn't get saved as 0 but remains a null

beautiful! 😍

we're not creating a library that people might use and care about versioning. it's just internal CLI that likely will keep evolving without care for BC or versioning. i'd rather not set this value and pretend that we're gonna properly version the commands 😅

this value seems to be used when displaying help message, and it says now that usage is pp-cli <command>, which i don't think is a valid command.

.scriptName('pnpm cli') makes help message render pnpm cli <command>, which is way more helpful

When trying to run it, I'm running into unexpected issues. The options don't seem to propagate to the subcommands? Maybe I'm running something a wrong way, but the second command is literally copied from the README 😅

andrea@andrea-black PronounsPage % pnpm cli translation create --locale fr

> [email protected] cli /Users/andrea/Projects/PronounsPage
> IS_CLI=true tsx --tsconfig .nuxt/tsconfig.server.json --import ./sentry.scripts.config.ts --env-file=.env bin/index.ts translation create --locale fr

Missing required option: locale
 ELIFECYCLE  Command failed with exit code 1.
andrea@andrea-black PronounsPage % pnpm cli translation merge -l fr -f merge.suml

> [email protected] cli /Users/andrea/Projects/PronounsPage
> IS_CLI=true tsx --tsconfig .nuxt/tsconfig.server.json --import ./sentry.scripts.config.ts --env-file=.env bin/index.ts translation merge -l fr -f merge.suml

Missing required option: locale
 ELIFECYCLE  Command failed with exit code 1.

But that aside… It looks like a big step in the right direction and a lot of good work! However, I'm afraid that we're also introducing a lot of complexity that will need to be maintained and understood for a long time, mainly to support zod typing for options that are only used by like five commands anyway 😅

I'm not sure how my imagined simple solution would look like exactly, but I'd like to give it a go before this PR gets merged, please give me a few hours to play around and propose an alternative before putting significant effort into this branch 😉

Andrea Vos (298a1c5e) at 19 Mar 12:27

... and 1 more commit