-
Notifications
You must be signed in to change notification settings - Fork 308
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
Simplify Record.__getitem__ / remove _bound_data #1158
Comments
The reason for Some data values need access to the record they are attached to. The data in I think the thinking is that the call to The only descriptor-valued types implemented in Lektor itself are
An alternative way around that would be to implement a |
Ah yes, of course. I hadn't considered this. So I want to ask then, "is the speedup relevant in any of these?" but seeing as new types are regularly introduced, the cautious thing to do is to assume the caching matters. I had considered a |
The Record class provides a handy use of
__getitem__
, but it's implementation introduces a complexity that does not need to be present. The method looks like this:lektor/lektor/db.py
Lines 473 to 481 in bfce91a
Apart from being initialized, this
_bound_data
is only used here. FlowBlock has a similar implmentation.As implemented,
_bound_data
provides a bit of a short circuit, and a record of what was accessed previously. The short circuit does not seem necessary to me, and I am unaware of any real use of distinguishing between_data
and_bound_data
. I even struggle to think of a contrived one, beyond a possible efficiency gain by having the short circuit. So I'd like to clean this up a bit and remove the concept of_bound_data
in Record and FlowBlock.Why not leave well enough alone?
I actually have a plugin that would attempt to alter a Record of FlowBlock's underlying data, but this is made much more convoluted when I try to account for
_bound_data
. I was inclined for a while to prefer handling that complexity in the plugin, but the more I linger on the topic, the more I'm thinking unnecessary complexity should probably be removed upstream if possible, especially in db.py. I intend to PR with a simplification, but I thought I'd post first to check that someone was opposed to the change. So far as I can tell, this is unused in the project and in any plugin I've checked, and it's a private attribute anyway.The text was updated successfully, but these errors were encountered: