Address review comments: fix logging, remove unused imports, fix error message, fix needs init, add html escaping, fix img tags, add tests

Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>

Author: Copilot

asimov/analysis.py

@@ -24,6 +24,7 @@
 """
 
 import os
+import html
 import configparser
 from copy import deepcopy
 import pathlib
@@ -1361,7 +1362,9 @@ def __init__(self, name, pipeline, ledger=None, **kwargs):
             # except KeyError:
             self.logger.warning(f"The pipeline {pipeline} could not be found.")
 
-        needs_value = self.meta.pop("needs", None)
+        # Read 'needs' from kwargs before it gets merged into self.meta so we
+        # don't accidentally mutate the class-level meta_defaults dict.
+        needs_value = kwargs.pop("needs", None)
         self._needs = cast(List[Any], needs_value) if needs_value is not None else []
 
         if "comment" in kwargs:
@@ -1662,14 +1665,15 @@ def html(self):
         # Status badge mapping
         status_badge = status_map.get(self.status, "secondary")
 
+        safe_name = html.escape(self.name)
         card = f"""
-<div class='project-analysis-card card event-data' id='project-{self.name}'>
+<div class='project-analysis-card card event-data' id='project-{safe_name}'>
     <div class='card-header'>
-        <h3 class='card-title'>{self.name}</h3>
+        <h3 class='card-title'>{safe_name}</h3>
 """
 
         if self.comment:
-            card += f"        <p class='text-muted'>{self.comment}</p>\n"
+            card += f"        <p class='text-muted'>{html.escape(self.comment)}</p>\n"
 
         # Status badge
         card += f"""        <span class='badge badge-{status_badge}'>{self.status}</span>

asimov/cli/application.py

@@ -229,7 +229,7 @@ def apply_page(file, event=None, ledger=None, update_page=False, name=None, iter
                 except KeyError as e:
                     click.echo(
                         click.style("●", fg="red")
-                        + f" Could not apply a production, couldn't find the event {event}"
+                        + f" Could not apply a production, couldn't find the event {event_s}"
                     )
                     logger.exception(e)
                     continue

asimov/event.py

@@ -678,16 +678,10 @@ def get_review_indicator(review_status):
                             rundir = node.rundir if hasattr(node, 'rundir') and node.rundir else ''
                             approximant = node.meta.get('approximant', '') if hasattr(node, 'meta') else ''
 
-                            # Get webdir for results links
-                            webdir = ''
-                            if hasattr(node, 'event') and hasattr(node.event, 'webdir') and node.event.webdir:
-                                webdir = node.event.webdir
-                            
                             # Construct potential result page URLs based on pipeline
                             result_pages = []
 
                             # Construct base URL using event name and analysis name
-                            import os
                             base_url = f"{self.name}/{node.name}"
 
                             # Add common result page patterns for different pipelines
@@ -817,7 +811,6 @@ def get_review_indicator(review_status):
                             rundir = node.rundir if hasattr(node, 'rundir') and node.rundir else ''
                             approximant = node.meta.get('approximant', '') if hasattr(node, 'meta') else ''
 
-                            import os
                             base_url = f"{self.name}/{node.name}"
 
                             result_pages = []

asimov/pipelines/__init__.py

@@ -1,3 +1,4 @@
+import logging
 import sys
 
 if sys.version_info < (3, 10):
@@ -14,6 +15,8 @@
 
 discovered_pipelines = entry_points(group="asimov.pipelines")
 
+logger = logging.getLogger(__name__)
+
 
 known_pipelines = {
     "bayeswave": BayesWave,
@@ -27,6 +30,10 @@
 for pipeline in discovered_pipelines:
     try:
         known_pipelines[pipeline.name] = pipeline.load()
-    except (ModuleNotFoundError, ImportError):
-        # Skip pipelines that can't be imported (e.g., testing pipelines in production)
-        pass
+    except (ModuleNotFoundError, ImportError) as e:
+        logger.warning(
+            "Could not load pipeline entry point %r: %s. "
+            "This pipeline will not be available.",
+            pipeline.name,
+            e,
+        )

asimov/pipelines/pesummary.py

@@ -602,13 +602,13 @@ def html(self):
 
             # Add common plot images that PESummary generates
             # Corner plot
-            out += f"""<img height=200 src="{pages_dir}/plots/corner.png" alt="Corner plot"></src>"""
+            out += f"""<img height=200 src="{pages_dir}/plots/corner.png" alt="Corner plot" />"""
 
             # Skymap
-            out += f"""<img height=200 src="{pages_dir}/plots/skymap.png" alt="Sky localization"></src>"""
+            out += f"""<img height=200 src="{pages_dir}/plots/skymap.png" alt="Sky localization" />"""
 
             # Waveform plots
-            out += f"""<img height=200 src="{pages_dir}/plots/waveform_time_domain_H1L1.png" alt="Waveform"></src>"""
+            out += f"""<img height=200 src="{pages_dir}/plots/waveform_time_domain_H1L1.png" alt="Waveform" />"""
 
             out += """</div>"""
 

tests/test_application.py

@@ -245,3 +245,75 @@ def test_multi_parameter_strategy_matrix(self):
                 self.assertEqual(prod.meta["sampler"]["sampler"], "dynesty")
             elif "emcee" in prod.name:
                 self.assertEqual(prod.meta["sampler"]["sampler"], "emcee")
+
+
+class NameIterateTests(AsimovTestCase):
+    """
+    Tests for the --name and --iterate options of apply_page.
+    """
+
+    def setUp(self):
+        super().setUp()
+        # Add the base event and a first analysis so we have an existing name
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_event.yaml",
+            ledger=self.ledger,
+        )
+
+    def test_name_overrides_blueprint_name(self):
+        """--name should replace the name field in the blueprint."""
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_analysis_S000000.yaml",
+            event="S000000",
+            ledger=self.ledger,
+            name="my-custom-name",
+        )
+        event = self.ledger.get_event("S000000")[0]
+        analysis_names = {prod.name for prod in event.productions}
+        self.assertIn("my-custom-name", analysis_names)
+        self.assertNotIn("bilby-IMRPhenomXPHM-QuickTest", analysis_names)
+
+    def test_iterate_increments_name_when_collision(self):
+        """--iterate should produce a -2 suffix when the base name is already taken."""
+        # Apply the analysis once to register the base name
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_analysis_S000000.yaml",
+            event="S000000",
+            ledger=self.ledger,
+        )
+        # Apply again with --iterate; a -2 suffix should be chosen
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_analysis_S000000.yaml",
+            event="S000000",
+            ledger=self.ledger,
+            iterate=True,
+        )
+        event = self.ledger.get_event("S000000")[0]
+        analysis_names = {prod.name for prod in event.productions}
+        self.assertIn("bilby-IMRPhenomXPHM-QuickTest", analysis_names)
+        self.assertIn("bilby-IMRPhenomXPHM-QuickTest-2", analysis_names)
+
+    def test_iterate_increments_correctly_for_strategy(self):
+        """--iterate with a strategy should give deterministic -2, -3, ... names."""
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_strategy_single.yaml",
+            event="S000000",
+            ledger=self.ledger,
+        )
+        # Apply the same strategy again with --iterate
+        apply_page(
+            f"{self.cwd}/tests/test_data/test_strategy_single.yaml",
+            event="S000000",
+            ledger=self.ledger,
+            iterate=True,
+        )
+        event = self.ledger.get_event("S000000")[0]
+        analysis_names = {prod.name for prod in event.productions}
+        # First pass
+        self.assertIn("bilby-IMRPhenomXPHM", analysis_names)
+        self.assertIn("bilby-SEOBNRv4PHM", analysis_names)
+        self.assertIn("bilby-IMRPhenomD", analysis_names)
+        # Second pass - iterated names
+        self.assertIn("bilby-IMRPhenomXPHM-2", analysis_names)
+        self.assertIn("bilby-SEOBNRv4PHM-2", analysis_names)
+        self.assertIn("bilby-IMRPhenomD-2", analysis_names)