diff --git a/nextstrain/cli/command/run.py b/nextstrain/cli/command/run.py index 2e4733db..bc3a8045 100644 --- a/nextstrain/cli/command/run.py +++ b/nextstrain/cli/command/run.py @@ -100,15 +100,6 @@ def run(opts): pathogen_directory = pathogen.path workflow_directory = pathogen.workflow_path(opts.workflow) - if not pathogen_directory.is_dir(): - raise UserError(f""" - No pathogen {opts.pathogen!r} found {f"in {str(pathogen_directory)!r}" if DEBUGGING else "locally"}. - - Did you set it up? - - Hint: to set it up, run `nextstrain setup {shquote(opts.pathogen)}`. - """) - if not workflow_directory.is_dir(): raise UserError(f""" No {opts.workflow!r} workflow for pathogen {opts.pathogen!r} found {f"in {str(workflow_directory)!r}" if DEBUGGING else "locally"}. diff --git a/nextstrain/cli/command/update.py b/nextstrain/cli/command/update.py index c09c59fe..7aa3d79a 100644 --- a/nextstrain/cli/command/update.py +++ b/nextstrain/cli/command/update.py @@ -15,8 +15,9 @@ """ from functools import partial -from textwrap import dedent +from textwrap import dedent, indent from ..argparse import SKIP_AUTO_DEFAULT_IN_HELP +from ..errors import UserError from ..util import colored, check_for_new_version, prose_list, runner_name from ..runner import all_runners_by_name, default_runner, runner_module from ..workflows import all_pathogen_defaults, PathogenWorkflows @@ -26,7 +27,7 @@ pathogens = all_pathogen_defaults() __doc__ = (__doc__ or "").format( - default_pathogens = prose_list([f"``{x}``" for x in pathogens], "and") if pathogens else "none", + default_pathogens = prose_list([f"``{x.name}``" for x in pathogens], "and") if pathogens else "none", default_runner_name = runner_name(default_runner), ) diff --git a/nextstrain/cli/util.py b/nextstrain/cli/util.py index 9170ec1f..ec30c0af 100644 --- a/nextstrain/cli/util.py +++ b/nextstrain/cli/util.py @@ -672,7 +672,7 @@ def prose_list(iterable: Iterable[str], conjunction: str = "or") -> str: return f" {conjunction} ".join(values) -def parse_version(version: str) -> Version: +def parse_version(version: str) -> 'LaxVersion': """ Parse *version* into a PEP-440-compliant :cls:`Version` object, by hook or by crook. @@ -700,7 +700,7 @@ def parse_version(version: str) -> Version: True """ try: - return Version(version) + return LaxVersion(version, compliant = True) except InvalidVersion: # Per PEP-440 # @@ -729,4 +729,14 @@ def parse_version(version: str) -> Version: as_local_segment = lambda v: replace_invalid_with_separators(remove_invalid_start_end_chars(v)) - return Version(f"0.dev0+{as_local_segment(version)}") + return LaxVersion(f"0.dev0+{as_local_segment(version)}", compliant = False, original = version) + + +class LaxVersion(Version): + compliant: bool + original: str + + def __init__(self, version: str, *, compliant: bool, original: str = None): + super().__init__(version) + self.compliant = compliant + self.original = original if original is not None else version diff --git a/nextstrain/cli/workflows.py b/nextstrain/cli/workflows.py index 5d3fe3a8..e39a1dcc 100644 --- a/nextstrain/cli/workflows.py +++ b/nextstrain/cli/workflows.py @@ -28,6 +28,19 @@ from .util import parse_version +# XXX TODO: I'm not very happy with the entirety of the conceptual organization +# and physical code organization in this file—in particular 1) the new_setup +# flag for handling of new vs. existing setups, 2) the tension between the main +# PathogenWorkflows class and the surrounding functions, and 3) the way the +# PathogenWorkflows constructor inconsistently cares about what's on the +# filesystem or not (e.g. for defaults)—but it does the job for now. If it +# continues to feel ill-fitting for the various uses, I suspect I'll end up +# reorganizing it after a while. This is also to say, if you're working in +# this code and have ideas for improving its organization, please do +# suggest/discuss them! +# -trs, 15 Jan 2025 + + class PathogenSpec(NamedTuple): # XXX FIXME: docstring """ @@ -108,11 +121,16 @@ def __init__(self, name_version_url: str, new_setup: bool = False): name, version, url = PathogenSpec.parse(name_version_url) if not name: - raise UserError(f""" - No name specified in {name_version_url!r}. + if new_setup: + raise UserError(f""" + No name specified in {name_version_url!r}. - All pathogen setups must be given a name, e.g. as in NAME[@VERSION[=URL]]. - """) + All pathogen setups must be given a name, e.g. as in NAME[@VERSION[=URL]]. + """) + else: + raise UserError(f""" + No pathogen name specified in {name_version_url!r}. + """) if disallowed := set([os.path.sep, os.path.altsep]) & set(name): raise UserError(f""" @@ -157,22 +175,31 @@ def __init__(self, name_version_url: str, new_setup: bool = False): specified, e.g. as in NAME@VERSION. """) else: - raise UserError(f""" - No version specified in {name_version_url!r}. + if versions := pathogen_versions(name): + raise UserError(f""" + No version specified in {name_version_url!r}. + + There's no default version set (or intuitable), so a version + must be specified, e.g. as in NAME@VERSION. + + Existing versions of {name!r} you have set up are: - There's no default version set (or intuitable), so a version - must be specified, e.g. as in NAME@VERSION. + {{versions}} - Existing versions of {name!r} you have set up are: + Hint: You can set a default version for {name!r} by running: - {{versions}} + nextstrain setup --set-default {shquote(name)}@VERSION - Hint: You can set a default version for {name!r} by running: + if you don't want to specify an explicit version every time. + """, versions = indent("\n".join(f"{name}@{v}" for v in versions), " ")) + else: + raise UserError(f""" + No pathogen setup exists for {name_version_url!r}. - nextstrain setup --set-default {shquote(name)}@VERSION + Did you set it up yet? - if you don't want to specify an explicit version every time. - """, versions = indent("\n".join(pathogen_versions(name)), " ")) + Hint: to set it up, run `nextstrain setup {shquote(name_version_url)}`. + """) if new_setup: if not url: @@ -200,6 +227,27 @@ def __init__(self, name_version_url: str, new_setup: bool = False): assert self.name assert self.version + if not new_setup: + if not self.path.is_dir(): + if versions := pathogen_versions(name): + raise UserError(f""" + No pathogen setup exists for {name_version_url!r}{f" in {str(self.path)!r}" if DEBUGGING else ""}. + + Existing versions of {name!r} you have set up are: + + {{versions}} + + Did you mean one of those? + """, versions = indent("\n".join(f"{name}@{v}" for v in versions), " ")) + else: + raise UserError(f""" + No pathogen setup exists for {name_version_url!r}{f" in {str(self.path)!r}" if DEBUGGING else ""}. + + Did you set it up yet? + + Hint: to set it up, run `nextstrain setup {shquote(name_version_url)}`. + """) + def __str__(self) -> str: return f"{self.name}@{self.version}" @@ -274,12 +322,12 @@ def setup(self, dry_run: bool = False, force: bool = False) -> SetupStatus: # XXX FIXME: dry_run if not force and self.path.exists(): print(f"Using existing setup in {str(self.path)!r}.") - print(f" Hint: if you want to ignore this existing installation, re-run `nextstrain setup` with --force.") + print(f" Hint: if you want to ignore this existing setup, re-run `nextstrain setup` with --force.") return True if self.path.exists(): assert force - print(f"Removing existing directory {str(self.path)!r} to start fresh…") + print(f"Removing existing setup {str(self.path)!r} to start fresh…") if not dry_run: rmtree(str(self.path)) @@ -419,7 +467,7 @@ def test_compatibility() -> SetupTestResult: return [ ('downloaded', - self.path.exists()), + self.path.is_dir()), ('contains nextstrain-pathogen.yaml', self.registration_path.is_file()), @@ -495,13 +543,19 @@ def pathogen_versions(name: str) -> List[str]: """ try: versions = [ - PathogenWorkflows._decode_version_dir(d.name) + parse_version(PathogenWorkflows._decode_version_dir(d.name)) for d in (WORKFLOWS / name).iterdir() if d.is_dir() ] except FileNotFoundError: versions = [] - return sorted(versions, key = parse_version, reverse = True) + # Sort newest → oldest for normal versions (e.g. 4.5.6, 1.2.3) and A → Z + # for non-compliant versions (e.g. branch names, commit ids, arbitrary + # strings, etc.), with the latter always after the former. + compliant = sorted(v for v in versions if v.compliant) + non_compliant = sorted(v for v in versions if not v.compliant) + + return [v.original for v in [*reversed(compliant), *non_compliant]] def pathogen_defaults(name: str) -> Tuple[Optional[PathogenWorkflows], Optional[PathogenWorkflows]]: