Makes article properties over writable by parser #330

MaxDall · 2023-12-10T13:33:14Z

This PR adds functionality to write over properties like lang defined in the article class. We do so by replacing the former property logic with a custom one. The new implementation now caches the values as well so it now works like cached_property but instead of cached_property it can be overwritten.

I.e. to overwrite the lang attribute of Article use the following in your desired parser.

from fundus.parser import ..., overwrite_attribute

    ...

    @overwrite_attribute
    def lang(self) -> str:
        return "This a new language value"

closes #328

src/fundus/parser/base_parser.py

dobbersc · 2023-12-14T17:28:02Z

src/fundus/parser/base_parser.py

-    def __init__(self, func: Callable[[object], Any], priority: Optional[int], validate: bool):
-        self.validate = validate
+    def __init__(self, func: Callable[[object], Any], priority: Optional[int], validate: bool, overwrite: bool = False):
+        self.validate = validate if not overwrite else False


Just writing the true path first for clarity.

Suggested change

self.validate = validate if not overwrite else False

self.validate = False if overwrite else validate

dobbersc · 2023-12-14T17:31:54Z

src/fundus/parser/base_parser.py

@@ -137,7 +145,7 @@ def validated(self) -> "AttributeCollection":

    @property
    def unvalidated(self) -> "AttributeCollection":
-        return AttributeCollection(*[attr for attr in self.functions if not attr.validate])
+        return AttributeCollection(*[attr for attr in self.functions if not attr.validate and not attr.overwrite])


Doesn't the following line modify the validated attribute already, s.t. it correctly identifies (un)validated attributes?

fundus/src/fundus/parser/base_parser.py

Line 80 in 1e506e1

self.validate = validate if not overwrite else False

Suggested change

return AttributeCollection(*[attr for attr in self.functions if not attr.validate and not attr.overwrite])

return AttributeCollection(*[attr for attr in self.functions if not attr.validate])

dobbersc · 2023-12-14T17:48:00Z

src/fundus/utils/caching.py

+import functools
+
+
+class _CachedAttribute(object):


In general, I was a bit confused with the features added in this PR. Firstly, the overwrite_attribute decorator and, secondly, the property caching. I'll spare you with my previous comments of confusion, but at first I thought that these are separate features and the custom cached attribute is only motivated to improve the general perfomance. Nertheless, here is how I understand it now:

The original problem outlined in #328 is caused by the use of the @property decorator on lang that we want to overwrite in the parsers. Here, we cannot continue using the standard properties since the defined properties define no setter, thus preventing the properties to be overwritten in subclasses with methods with the @attribute decorator. Did I understand this correctly? The origin of the error or at least an error traceback would have been helpful to include in the issue or the PR description.

dobbersc · 2023-12-14T18:10:51Z

src/fundus/parser/base_parser.py

+    def __init__(self, func: Callable[[object], Any], priority: Optional[int], validate: bool, overwrite: bool = False):
+        self.validate = validate if not overwrite else False
+        self.overwrite = overwrite


Could you clarify why an overwritten attribute can not be a validated attribute? If we allow overwritten attributes to be validated, they are the same as the regular attributes.

dobbersc · 2023-12-14T19:20:56Z

src/fundus/utils/caching.py

+import functools
+
+
+class _CachedAttribute(object):


I'm a bit unsure if this entire attribute caching via a custom property is necessary. Some alternatives come to my mind:

We could define a setter for the relevant properties. But this would be bad since we don't want lang etc. to be mutable openly.

From a design perspective, why are the lang and plaintext computed in the article and not in the parser? So maybe we can move both of these properties from the Article to the BaseParser as attributes. Then we need to include lang and plaintext as fields in the Article dataclass, s.t. they are populated delayed as the other attributes. This way it would also be explicit that these are attributes one may overwrite from the BaseParser and Article would define the skeleton of attributes as dataclass fields.

What would be wrong with the regular cached_attribute? I tried to replace it with the custom one and it seemed to work fine, even when overwriting it in a parser.

MaxDall added 2 commits December 10, 2023 14:24

makes article properties overridable by parsers

a626137

expose overwrite_attribute in base_parser.py

1e506e1

dobbersc requested changes Dec 14, 2023

View reviewed changes

remove leftover try:except

85a2d68

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes article properties over writable by parser #330

Makes article properties over writable by parser #330

MaxDall commented Dec 10, 2023 •

edited

dobbersc Dec 14, 2023

dobbersc Dec 14, 2023

dobbersc Dec 14, 2023

dobbersc Dec 14, 2023

dobbersc Dec 14, 2023

	self.validate = validate if not overwrite else False
	self.validate = False if overwrite else validate

	return AttributeCollection(*[attr for attr in self.functions if not attr.validate and not attr.overwrite])
	return AttributeCollection(*[attr for attr in self.functions if not attr.validate])

Makes article properties over writable by parser #330

Are you sure you want to change the base?

Makes article properties over writable by parser #330

Conversation

MaxDall commented Dec 10, 2023 • edited

dobbersc Dec 14, 2023

Choose a reason for hiding this comment

dobbersc Dec 14, 2023

Choose a reason for hiding this comment

dobbersc Dec 14, 2023

Choose a reason for hiding this comment

dobbersc Dec 14, 2023

Choose a reason for hiding this comment

dobbersc Dec 14, 2023

Choose a reason for hiding this comment

MaxDall commented Dec 10, 2023 •

edited