Closed MichaelMauderer closed 2 weeks ago
Yes, please especially for the separate module. Great works.
Fred
On 24 Sep 2024, at 09:22, Thomas Mansencal @.***> wrote:
@KelSolaar commented on this pull request.
Thanks you! This looks great overall. The main two comments I would have are:
Maybe considering using an ABC dataclass so that we can consistently expose from_xml and a future to_xml I would move all the utility functions to a separate common/utilities module so that the core of the code stays neat and tidy. In colour/io/clf.py https://github.com/colour-science/colour/pull/1278#discussion_r1772714738:
- if config.namespace_name is None:
- return name
- else:
- return f"{{{config.namespace_name}}}{name}"
+def map_optional(f: Callable, value):
- if value is not None:
- return f(value)
- return None
+def retrieve_attributes(
- xml, attribute_mapping: dict[str, str] +) -> dict[str, str | None]:
- def get_attribute(name): Minor
Do we need this function here instead of than calling directly xml.get(attribute_name)?
In colour/io/clf.py https://github.com/colour-science/colour/pull/1278#discussion_r1772716562:
- def get_attribute(name):
- return xml.get(name)
- return {
- k: get_attribute(attribute_name)
- for k, attribute_name in attribute_mapping.items()
- }
+def retrieve_attributes_as_float(
- xml, attribute_mapping: dict[str, str] +) -> dict[str, float | None]:
- attributes = retrieve_attributes(xml, attribute_mapping)
- def as_float(value):
- if value is None: Minor
At the point we are already leveraging an exception, I would probably remove this clause and use except (TypeError, ValueError): instead.
In colour/io/clf.py https://github.com/colour-science/colour/pull/1278#discussion_r1772736898:
- Expects the xml element to be a valid element according to the CLF
- specification.
- Raises
- :class: CLFValidationError
- If the node does not conform to the specification, a
CLFValidationError
- will be raised. The error message will indicate the details of the issue
- that was encountered.
- """
- if xml is None:
- return None
- super_args = ProcessNode.parse_attributes(xml, config)
- style = map_optional(ExponentStyle, xml.get("style"))
- if style is None:
- raise CLFValidationError("Exponent process node has no `style' value.") Pedantry
The quotes are mismatched here:
`style'
In colour/io/clf.py https://github.com/colour-science/colour/pull/1278#discussion_r1772739339:
@.*** +class ExponentParams:
- """
- Represents a Exponent Params element.
- References
- https://docs.acescentral.com/specifications/clf/#exponent
- """
- exponent: float
- offset: float | None
- channel: Channel | None
- @staticmethod
- def from_xml(xml, _config: ParserConfig): Major
Is there a reason for _config arg here with underscore when the other ones are using config, I'm assuming that it is because it is not used?
This makes me think that we should maybe look at an ABC dataclass so that we can enforce the from_xml signature consistently everywhere.
In colour/io/clf.py https://github.com/colour-science/colour/pull/1278#discussion_r1772766149:
- else:
- return xml.xpath(f"{name}/text()")
+processing_node_constructors = {}
+def register_process_node_xml_constructor(name):
- def register(constructor):
- processing_node_constructors[name] = constructor
- return constructor
- return register
+def valid_processing_node_tags(): Minor
This definition is not used currently.
— Reply to this email directly, view it on GitHub https://github.com/colour-science/colour/pull/1278#pullrequestreview-2324204746, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABHZDWXYCFS3H7VLUDNFM6TZYEAJPAVCNFSM6AAAAABKTW6XNOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRUGIYDINZUGY. You are receiving this because you are subscribed to this thread.
Closed in favour of https://github.com/colour-science/colour-clf-io
Summary
Adds a module that allows contains the functionality and data structures to parse CLF files. This is the first step to add full support for CLF into colour.
Preflight
Code Style and Quality
colour
,colour.models
.Documentation
More docs and examples to come in follow-up PRs, once we can do more than parse the CLF files.