Generate XLSX from schemapack (GSI-676) #144
Conversation
There was a problem hiding this comment.
Should we adhere to the practice of including docstrings for every function and class, or is it acceptable to omit them when the function is self-explanatory?
In addition, there are some inline comments and suggestions. Furthermore, I've included the submission.schemapack.yaml file in the repository to resolve failing checks, except for the linkml-specific schema-linter. We may consider removing the schema-linter, as it's no longer relevant. Or replace it with a schemapack-specific linter. However, I've implemented a temporary solution to utilize only the linkml schema, so keeping it shouldn't pose any issues for this PR.
| from openpyxl.utils import get_column_letter | ||
| from pydantic import BaseModel, root_validator | ||
| import yaml | ||
| from pydantic import BaseModel |
There was a problem hiding this comment.
| from pydantic import BaseModel | |
| from pydantic import BaseModel, Field |
scripts/generate_xlsx.py
Outdated
| """A XLSX generator config""" | ||
|
|
||
| output_filename: str | ||
| styles: Optional[dict[str, WorksheetStyle]] = {} |
There was a problem hiding this comment.
| styles: Optional[dict[str, WorksheetStyle]] = {} | |
| styles: dict[str, WorksheetStyle] = Field(default_factory=dict) |
There was a problem hiding this comment.
I agree with the obsolete Optional but replace the plain default with a Field() and a factory?
| header_color: Optional[str] = None | ||
| content_color: Optional[str] = None |
There was a problem hiding this comment.
Header and content color is defined as the attributes of the WorksheetStyle class previously. Maybe this would be better:
| header_color: Optional[str] = None | |
| content_color: Optional[str] = None | |
| style: Optional[WorksheetStyle] = None |
| class Column: | ||
| name: str | ||
| description: Optional[str] | ||
| type: str |
There was a problem hiding this comment.
Column type can be an enum, by that we would actually have control over the vocabulary. Smt like:
class ValueType(Enum): ARRAY = "array" ENUM = "enum" STRING = "string"
| def _get_element_type_from_schema(schema: dict) -> str: | ||
| """Get the type of the elements in an array from a JSON schema.""" | ||
| element_type = schema.get("type", "object") | ||
| if element_type == "array": |
There was a problem hiding this comment.
If you accept the previous enum idea:
| if element_type == "array": | |
| if element_type == ValueType.ARRAY.value: |
|
|
||
| cols = sheet.columns | ||
| rows = [] | ||
| rows.append([Cell(wb_sheet, value=col.name) for col in cols]) |
There was a problem hiding this comment.
I get type error type error from all the Cell related operations. Shall we suppress the with # type: ignore? I do not know if there is a way to suppress all the Cell related type errors through out this script.
Co-authored-by: sbilge <[email protected]>
Co-authored-by: sbilge <[email protected]>
sbilge
force-pushed
the
refactor_schemapack
branch
from
441a5e5 to
a4afdf7
Compare
This PR is a refactoring of the XLSX generator script to use schemapack instead of LinkML. Other than that the behaviour changes as follows: