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

Add opticks cli #75

Merged
merged 33 commits into from
Mar 21, 2024
Merged

Add opticks cli #75

merged 33 commits into from
Mar 21, 2024

Conversation

dale-french
Copy link
Collaborator

@dale-french dale-french commented Feb 28, 2024

Description

This PR does the following:

  • Migrates the project into a Yarn Workspaces monorepo and introduces two packages, namely:
    • lib: The existing opticks library
    • cli: A new CLI called opticks-cli which handles automated cleanup of toggle code
  • Removes JsCodeShift scripts from opticks

CLI

This is a first version of the CLI so does not come with all the bells and whistles (YET!). At the moment the CLI takes care of running JsCodeShift codemods in a way that is more user friendly than the existing scripts. In the future, this CLI will also be able to take care of removing unused files and dependencies used on the cleaned up side of the experiment using Knip.

At the moment, the CLI looks like this:

Without options specified

CleanShot 2024-02-29 at 14 44 21

With options specified

CleanShot 2024-02-29 at 14 46 29

Next steps

As this is a first version, I would like to test this in the real-world before making any additional improvements. Upcoming improvements would look something like this:

  • Publish both packages using changesets - currently only opticks is published automatically
  • Add Knip to clean up unused files and dependencies
  • Update documentation for opticks and opticks-cli

@dale-french dale-french self-assigned this Feb 28, 2024
type: 'select',
name: 'winner',
message: 'Please enter the winning side of the experiment:',
choices: ['A', 'B']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically opticks supports a, b, c, d, etc https://github.com/viodotcom/opticks/blob/master/src/core/toggle.ts#L11

Should we allow it here too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think for ease in the beginning we keep it as is and expand if we think its necessary (I think there has only been 1 MVT since I joined?) - we can always use the full command if needed:

opticks-cli clean --id=ab-test-123 --winner=c

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK fair enough, though I think we should design it to be flexible, optimizing for the most used case is fine here.

Copy link
Collaborator

@jopdeklein jopdeklein left a comment

Choose a reason for hiding this comment

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

Thanks for making the clean up process a lot nicer, @dale-french 🎉

type: 'select',
name: 'winner',
message: 'Please enter the winning side of the experiment:',
choices: ['A', 'B']
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK fair enough, though I think we should design it to be flexible, optimizing for the most used case is fine here.

@dale-french dale-french merged commit 69c5295 into master Mar 21, 2024
1 check passed
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.

2 participants