Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove function indirection for common options #552

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

ecooper
Copy link
Contributor

@ecooper ecooper commented Jan 6, 2025

Problem

While working on a future feature, I found myself confused/uncertain around what the variety of common option functions were doing, and then later, how to separate them by dependency--I needed account API options and database paths, but no core API related changes.

The only way I could accomplish what I wanted was by duplicating options. Feels bad.

Solution

Organize the common option sets by dependency. Today, we effectively have 4 common option sets:

  • Account API: Options required for commands with a dependency on the Account API. Commands using short-lived keys have a dependency on the Account API.
  • Core API: Options required for commands making a call to the Core API.
  • Query: Options required for commands calling the query endpoint of the Core API.
  • Database paths: Options for commands using database paths to reference a specific database. Arguably, this belongs in the account API set, but I split it out like the query options for explicitness.

I'm very open to changing how these are organized/naming conventions so long as we're explicitly adding each option set for a given command.

Result

Commands can opt-in to specific dependency-based option sets when defining their command:

yargs
    .options(ACCOUNT_OPTIONS)
    .options(DATABASE_PATH_OPTIONS)
    .options(CORE_OPTIONS)
    .command(abandonCommand)
    .command(commitCommand)
    .command(diffCommand)
    .command(pushCommand)
    .command(pullCommand)
    .command(statusCommand)
    .check(validateDatabaseOrSecret)
    .demandCommand();

As part of this work, the following changes were implemented:

  • Renamed command-helpers.mjs to options.mjs.
  • Moved middleware in command-helpers.mjs to middleware.mjs.
  • Renamed misc.mjs to utils.mjs and moved left-over functions from command-helpers.mjs into that.

Testing

Ran the existing test suite.

Copy link
Contributor

@ptpaterson ptpaterson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I like this refactor a lot.

@ecooper ecooper merged commit 57641e1 into main Jan 6, 2025
4 checks passed
@ecooper ecooper deleted the option-indirection branch January 6, 2025 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants