Skip to content

Conversation

@ajrbyers
Copy link
Contributor

No description provided.

model_name='bandingtypecurrencyentry',
index=models.Index(fields=['package', 'banding_type_entry'], name='btce_pkg_bte_idx'),
),
migrations.AddIndex(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure how helpful this index will be, as the bandingtypeentry is probably shared by many rows

else:
self.helper.layout.append(
Layout(
HTML(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat curious offloading the HTML generation into this routine, rather than using a template?

return prices

@functools.cached_property
def fast_price_bandings(self) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks OK, but I really would like it to have a debug/logging mode where it writes output about what it is doing to the price debugger. Could just add an optional parameter to the function debug=False that when true then returns a tuple of (prices, debug_string). This way we could easily hook it up to the debugger? I am just worried that, although it all looks fine at this point, we might hit an edge case with OBC where we can't work out what it's doing and need to debug the logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. This might be the wrong place for this comment. I can see that other functions still have the "explain" bool signature, below, so perhaps the debugger is still in tact

def _compare(self, package):
original = package.price_bandings

if "fast_price_bandings" in package.__dict__:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is forcing a cached property to reload, but it's hard to understand. It looks like the code deletes an instance attribute but then accesses it immediately...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants