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

CLI improvements #1611

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

CLI improvements #1611

wants to merge 6 commits into from

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Dec 6, 2022

Description of proposed changes

See commit messages.

Related issue(s)

Testing

@victorlin victorlin self-assigned this Dec 6, 2022
@nextstrain-bot nextstrain-bot temporarily deployed to auspice-victorlin-build-dsi554 December 6, 2022 01:14 Inactive
@victorlin victorlin mentioned this pull request Dec 6, 2022
7 tasks
@jameshadfield jameshadfield mentioned this pull request Dec 7, 2022
6 tasks
cli/view.js Outdated Show resolved Hide resolved
cli/view.js Outdated Show resolved Hide resolved
@@ -24,6 +24,7 @@ const addParser = (parser) => {
subparser.addArgument('--handlers', {action: "store", metavar: "JS", help: "Overwrite the provided server handlers for client requests. See documentation for more details."});
subparser.addArgument('--datasetDir', {metavar: "PATH", help: "Directory where datasets (JSONs) are sourced. This is ignored if you define custom handlers."});
subparser.addArgument('--narrativeDir', {metavar: "PATH", help: "Directory where narratives (Markdown files) are sourced. This is ignored if you define custom handlers."});
subparser.addArgument('--customBuildOnly', {action: "storeTrue", help: "Error if a custom build is not found."});
Copy link
Member

@jameshadfield jameshadfield Dec 7, 2022

Choose a reason for hiding this comment

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

I like the intention here but the word "custom" is a bit overloaded here -- this is checking if an auspice build is present in the current working directory, not whether the build has been customised (which would require introspection of the bundle).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've split the addition of this argument into its own commit: 0090265

Ok I understand the difference now. I chose --customBuildOnly following the previous existence of --customBuild, which seemed to be doing something similar? I can't think of a better name without being verbose like --onlyUseLocalAuspiceBuild. Do you have suggestions?

cli/view.js Outdated Show resolved Hide resolved
@victorlin victorlin force-pushed the victorlin/build-cli-improvements branch from b26aeeb to 0090265 Compare November 30, 2023 22:01
@victorlin victorlin marked this pull request as ready for review November 30, 2023 22:08
This logic is specific to build, so keep it there.
Updated the verbose logging to be technically correct:

- The index file is copied from the root to dist/, then served from
  there.
- Only favicon.png is served from the root given the subsequent lines of
  routing.
This has no effect other than a printed warning, and has been like this
since 55c69bb.
When running from Auspice's root directory, this replaces an unhandled
scandir error with a more meaningful message.
When specified, this prevents falling back to running from build files
in Auspice's root directory, which can be unintentional when running
from an external project.
@victorlin victorlin force-pushed the victorlin/build-cli-improvements branch from 0090265 to f51dff6 Compare November 30, 2023 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

3 participants