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

view: Support narratives #240

Merged
merged 6 commits into from
Dec 7, 2022
Merged

view: Support narratives #240

merged 6 commits into from
Dec 7, 2022

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Dec 6, 2022

See commit messages for details. The summaries:

  1. 6950f2f view: Support narratives
  2. 334ab6d view: Open a browser automatically
  3. 77f2975 view: Adjust dataset_paths() and narrative_paths() to take a list of file paths
  4. de0ef0c view: Accept paths to a specific dataset or narrative file

Related issue(s)

#128

Testing

  • Manually tested various invocations with a variety of circumstances
  • Tests pass locally
  • Checks pass

@tsibley tsibley requested a review from a team December 6, 2022 19:48
@tsibley
Copy link
Member Author

tsibley commented Dec 6, 2022

@huddlej You may be interested in this given you opened #128.

Previously the launched Auspice would either show baked in test
narratives or no narratives at all, depending on the Auspice version.

Wee bit of an oversight. orz

Resolves #128.
Supersedes #129.
Reduces friction and potential confusion e.g. over the URL reported by
Nextstrain CLI vs. the URL reported by Auspice.

If there's only a single dataset or narrative available, then open it
directly.  Otherwise, rely on Auspice's dataset/narrative listing page
for folks to get where they're going.
…file paths

…instead of a single directory path.  This will let us use them with
pre-filtered lists of files to see what's a dataset and what's a
narrative.
The data directories used by Auspice will be inferred from the file's
path and the given dataset or narrative will be opened in a browser by
default (instead of Auspice's dataset/narrative listing).

This makes the UX for viewing a specific dataset or narrative much
nicer, with fewer steps and a more direct translation of intent into
invocation (e.g. "I want to view this dataset" vs. "I want to view this
dataset, so I have to specify its parent directory").
@tsibley tsibley force-pushed the trs/view/narratives-take-two branch from de0ef0c to a0f3ad1 Compare December 6, 2022 19:50
Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

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

Very cool 🤩

nextstrain/cli/command/view.py Show resolved Hide resolved


# Avoid text-mode browsers
TERM = environ.pop("TERM", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

I went down a tiny rabbit hole of what TERM means. Finally found it being used in the webbrowser code for registering browers 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Also learned that webbrowser checks BROWSER for user specified browser. This works with nextstrain view to use a specific browser rather than the default!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, TERM is an old and very standard environment variable. BROWSER is a less old but still pretty conventional variable. That's, of course, why Python's webbrowser module uses them. :-)

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Looks good from reading through the code 👍

@tsibley tsibley merged commit 6c095c3 into master Dec 7, 2022
@tsibley tsibley deleted the trs/view/narratives-take-two branch December 7, 2022 18:52
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