-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Makes article properties over writable by parser #330
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,10 @@ | ||
from .base_parser import BaseParser, ParserProxy, attribute, function | ||
from .base_parser import ( | ||
BaseParser, | ||
ParserProxy, | ||
attribute, | ||
function, | ||
overwrite_attribute, | ||
) | ||
from .data import ArticleBody | ||
|
||
__all__ = ["ParserProxy", "BaseParser", "attribute", "function", "ArticleBody"] | ||
__all__ = ["ParserProxy", "BaseParser", "attribute", "function", "overwrite_attribute", "ArticleBody"] |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -76,8 +76,9 @@ def __repr__(self): | |||||||
|
||||||||
|
||||||||
class Attribute(RegisteredFunction): | ||||||||
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 | ||||||||
self.overwrite = overwrite | ||||||||
Comment on lines
+79
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
super(Attribute, self).__init__(func=func, priority=priority) | ||||||||
|
||||||||
|
||||||||
|
@@ -88,7 +89,10 @@ def __init__(self, func: Callable[[object], Any], priority: Optional[int]): | |||||||
|
||||||||
def _register(cls, factory: Type[RegisteredFunction], **kwargs): | ||||||||
def wrapper(func): | ||||||||
return functools.update_wrapper(factory(func, **kwargs), func) | ||||||||
try: | ||||||||
return functools.update_wrapper(factory(func, **kwargs), func) | ||||||||
except TypeError as err: | ||||||||
raise err | ||||||||
dobbersc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
||||||||
# _register was called with parenthesis | ||||||||
if cls is None: | ||||||||
|
@@ -102,6 +106,10 @@ def attribute(cls=None, /, *, priority: Optional[int] = None, validate: bool = T | |||||||
return _register(cls, factory=Attribute, priority=priority, validate=validate) | ||||||||
|
||||||||
|
||||||||
def overwrite_attribute(cls): | ||||||||
return _register(cls, factory=Attribute, priority=None, validate=False, overwrite=True) | ||||||||
|
||||||||
|
||||||||
def function(cls=None, /, *, priority: Optional[int] = None): | ||||||||
return _register(cls, factory=Function, priority=priority) | ||||||||
|
||||||||
|
@@ -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]) | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||
|
||||||||
|
||||||||
class FunctionCollection(RegisteredFunctionCollection[Function]): | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import functools | ||
|
||
|
||
class _CachedAttribute(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, I was a bit confused with the features added in this PR. Firstly, the The original problem outlined in #328 is caused by the use of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unsure if this entire attribute caching via a custom property is necessary. Some alternatives come to my mind:
|
||
"""Computes attribute value and caches it in the instance. | ||
From https://stackoverflow.com/questions/7388258/replace-property-for-perfomance-gain?noredirect=1&lq=1 | ||
Tweaked a bit to be used with a wrapper. | ||
""" | ||
|
||
def __init__(self, method): | ||
self.method = method | ||
|
||
def __get__(self, inst, cls): | ||
if inst is None: | ||
return self | ||
result = self.method(inst) | ||
object.__setattr__(inst, self.__name__, result) # type: ignore[attr-defined] | ||
return result | ||
|
||
|
||
# This was implemented in order to | ||
def cached_attribute(attribute): | ||
"""Decorate attributes to be cached. | ||
|
||
This works like `cached_property`, but instead of `property` or `cached_property`, the decorated attribute | ||
can be overwritten. | ||
|
||
Args: | ||
attribute: The attribute to decorate. | ||
|
||
Returns: | ||
A wrapped _CachedAttribute instance. | ||
|
||
""" | ||
|
||
def wrapper(func): | ||
return functools.update_wrapper(_CachedAttribute(func), func) | ||
|
||
return wrapper(attribute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just writing the true path first for clarity.