wip! streak's hoodwinked

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"}.

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),
 )
 

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

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]]: