Merge pull request #120 from lsst/tickets/DM-47256

DM-47256: Remove apply_schema_to_tables flag

docs/changes/DM-47256.removal.rst

@@ -0,0 +1,2 @@
+Remove the ``apply_schema_to_tables`` flag on the ``MetaDataBuilder`` which creates SQLAlchemy ``MetaData`` from a Felis ``Schema``.
+This is a redundant variable as enabling the ``apply_schema_to_metadata`` flag already correctly associates the metadata to the schema.

python/felis/db/schema.py

@@ -53,9 +53,8 @@ def create_database(schema: Schema, engine_or_url_str: Engine | str | None = Non
         )
     else:
         engine = create_engine("sqlite:///:memory:")
-    apply_schema = False if engine.url.drivername == "sqlite" else True
     metadata = MetaDataBuilder(
-        schema, apply_schema_to_metadata=apply_schema, apply_schema_to_tables=apply_schema
+        schema, apply_schema_to_metadata=False if engine.url.drivername == "sqlite" else True
     ).build()
     ctx = DatabaseContext(metadata, engine)
     ctx.initialize()

python/felis/diff.py

@@ -218,9 +218,7 @@ def __init__(self, schema: Schema, engine: Engine):
             mc = MigrationContext.configure(
                 connection, opts={"compare_type": True, "target_metadata": db_metadata}
             )
-            schema_metadata = MetaDataBuilder(
-                schema, apply_schema_to_metadata=False, apply_schema_to_tables=False
-            ).build()
+            schema_metadata = MetaDataBuilder(schema, apply_schema_to_metadata=False).build()
             self.diff = compare_metadata(mc, schema_metadata)
 
     def print(self) -> None:

python/felis/metadata.py

@@ -125,8 +125,6 @@ class MetaDataBuilder:
         The schema object from which to build the SQLAlchemy metadata.
     apply_schema_to_metadata
         Whether to apply the schema name to the metadata object.
-    apply_schema_to_tables
-        Whether to apply the schema name to the tables.
     ignore_constraints
         Whether to ignore constraints when building the metadata.
     """
@@ -135,18 +133,14 @@ def __init__(
         self,
         schema: Schema,
         apply_schema_to_metadata: bool = True,
-        apply_schema_to_tables: bool = True,
         ignore_constraints: bool = False,
     ) -> None:
         """Initialize the metadata builder."""
         self.schema = schema
         if not apply_schema_to_metadata:
             logger.debug("Schema name will not be applied to metadata")
-        if not apply_schema_to_tables:
-            logger.debug("Schema name will not be applied to tables")
         self.metadata = MetaData(schema=schema.name if apply_schema_to_metadata else None)
         self._objects: dict[str, Any] = {}
-        self.apply_schema_to_tables = apply_schema_to_tables
         self.ignore_constraints = ignore_constraints
 
     def build(self) -> MetaData:
@@ -235,7 +229,6 @@ def build_table(self, table_obj: datamodel.Table) -> None:
             self.metadata,
             *columns,
             comment=description,
-            schema=self.schema.name if self.apply_schema_to_tables else None,
             **optargs,  # type: ignore[arg-type]
         )
 

python/felis/tap_schema.py

@@ -131,9 +131,7 @@ def _load_yaml(self) -> None:
             self.schema_name = self.schema.name
 
         self._metadata = MetaDataBuilder(
-            self.schema,
-            apply_schema_to_metadata=self.apply_schema_to_metadata,
-            apply_schema_to_tables=self.apply_schema_to_metadata,
+            self.schema, apply_schema_to_metadata=self.apply_schema_to_metadata
         ).build()
 
         logger.debug("Loaded TAP_SCHEMA '%s' from YAML resource", self.schema_name)

tests/test_cli.py

@@ -207,9 +207,7 @@ def test_diff_database(self) -> None:
         db_url = f"sqlite:///{self.tmpdir}/tap_schema.sqlite3"
 
         engine = create_engine(db_url)
-        metadata_db = MetaDataBuilder(
-            Schema.from_uri(test_diff1), apply_schema_to_metadata=False, apply_schema_to_tables=False
-        ).build()
+        metadata_db = MetaDataBuilder(Schema.from_uri(test_diff1), apply_schema_to_metadata=False).build()
         metadata_db.create_all(engine)
 
         runner = CliRunner()

tests/test_diff.py

@@ -34,7 +34,7 @@ class TestSchemaDiff(unittest.TestCase):
     def _diff(self, schema1, schema2):
         return SchemaDiff(schema1, schema2).diff
 
-    def test_schema_diff(self):
+    def test_schema_diff(self) -> None:
         """Test the comparison output generated by the SchemaDiff class."""
         # Two schemas with different values
         schema1 = dm.Schema(name="schema1", id="#schema1", version="1.2.3", description="Schema 1", tables=[])
@@ -192,7 +192,7 @@ def test_schema_diff(self):
         # Call formatted handler function
         FormattedSchemaDiff(schema1, schema2)._handle_dictionary_item_removed(diff["dictionary_item_removed"])
 
-    def test_index_diff(self):
+    def test_index_diff(self) -> None:
         """Test differences in indices between tables."""
         schema1 = dm.Schema(name="schema1", id="#schema1", version="1.2.3", description="Schema 1", tables=[])
         schema1.tables.append(dm.Table(name="table1", id="#table1", description="Table 1", columns=[]))
@@ -222,22 +222,22 @@ def test_index_diff(self):
         # Print formatted diff to make sure it works for these changes
         FormattedSchemaDiff(schema1, schema2).print()
 
-    def test_print(self):
+    def test_print(self) -> None:
         schema1 = dm.Schema(name="schema1", id="#schema1", version="1.2.3", description="Schema 1", tables=[])
         schema2 = dm.Schema(name="schema2", id="#schema2", version="4.5.6", description="Schema 2", tables=[])
         SchemaDiff(schema1, schema2).print()
 
-    def test_formatted_print(self):
+    def test_formatted_print(self) -> None:
         schema1 = dm.Schema(name="schema1", id="#schema1", version="1.2.3", description="Schema 1", tables=[])
         schema2 = dm.Schema(name="schema2", id="#schema2", version="4.5.6", description="Schema 2", tables=[])
         FormattedSchemaDiff(schema1, schema2).print()
 
-    def test_parse_deepdiff_path(self):
+    def test_parse_deepdiff_path(self) -> None:
         path = "root['tables'][0]['columns'][0]['ivoa_ucd']"
         keys = FormattedSchemaDiff._parse_deepdiff_path(path)
         self.assertListEqual(keys, ["tables", 0, "columns", 0, "ivoa_ucd"])
 
-    def test_get_id_error(self):
+    def test_get_id_error(self) -> None:
         id_dict = {"tables": [{"indexes": [{"columns": [{"name": "column1"}, {"name": "column2"}]}]}]}
         keys = ["tables", 0, "indexes", 0, "columns", 0]
         with self.assertRaises(ValueError):
@@ -247,7 +247,7 @@ def test_get_id_error(self):
 class TestDatabaseDiff(unittest.TestCase):
     """Test the DatabaseDiff class."""
 
-    def test_database_diff(self):
+    def test_database_diff(self) -> None:
         """Test the comparison output generated by the DatabaseDiff class."""
         # Two tables with different columns
         schema1 = dm.Schema(name="schema1", id="#schema1", version="1.2.3", description="Schema 1", tables=[])
@@ -265,9 +265,7 @@ def test_database_diff(self):
             dm.Column(name="column2", datatype="string", length=256, id="#column2", description="Column 2")
         )
 
-        metadata_db = MetaDataBuilder(
-            schema1, apply_schema_to_metadata=False, apply_schema_to_tables=False
-        ).build()
+        metadata_db = MetaDataBuilder(schema1, apply_schema_to_metadata=False).build()
         engine = create_engine("sqlite:///:memory:")
         metadata_db.create_all(engine)
 

tests/test_metadata.py

@@ -147,7 +147,7 @@ def test_builder(self) -> None:
         builder matches the data in the Felis schema used to create it.
         """
         sch = Schema.model_validate(self.yaml_data)
-        bld = MetaDataBuilder(sch, apply_schema_to_tables=False, apply_schema_to_metadata=False)
+        bld = MetaDataBuilder(sch, apply_schema_to_metadata=False)
         md = bld.build()
 
         self.assertEqual(len(sch.tables), len(md.tables))