Compute TokenList.value dynamically #623
Conversation
This will allow moving some filters to run before the tokens are grouped.
Rename Token to TokenBase and make it a superclass for TokenList and a new Token class. Move some of the functionality of TokenBase into Token and TokenList.
The fact that a new value was being computed each time TokenList.group_tokens() is called caused supra-linear runtime when token grouping was enabled. Address by making TokenList.value a dynamically-computed property rather than a static attribute.
Codecov Report
@@ Coverage Diff @@
## master #623 +/- ##
==========================================
+ Coverage 96.82% 96.86% +0.03%
==========================================
Files 20 20
Lines 1514 1532 +18
==========================================
+ Hits 1466 1484 +18
Misses 48 48
Continue to review full report at Codecov.
|
|
|
||
| def enable_grouping(self): | ||
| self._grouping = True | ||
| self.grouping_filter.enable() |
There was a problem hiding this comment.
I was working off of this change today and found myself wanting to initialize it to None on line 20, then build the GroupingFilter and append it it to stmtprocess here if it is None.
There was a problem hiding this comment.
The reason I went with the approach that I did is that I wanted to ensure that grouping continued to happen before the other filters were run. Looking at the usages within sqlparse, implementing your approach would keep the existing behavior. I also know that Django Debug Toolbar calls .enable_grouping() manually, but it shouldn't be affected by your approach either.
The only concern would be if some other code appends a filter that requires grouping and then calls .enable_grouping() after appending that filter, things would break.
|
This PR will potentially fix django-commons/django-debug-toolbar#1402. I would really appreciate if it can be reviewed and considered for being merged! |
|
Closing in favor of #710. |
Partial fix for #621:
To avoid quadratic behavior, make
TokenList.valuea dynamically-computed property rather than an attribute so that it does not need to be recomputed each timeTokenList.group_tokens()is called withextend=True.The first three commits in this PR are supporting work: I found that making
TokenList.valuea property caused test failures due to line endings not being correctly preserved when stripping comments. This is due to the fact that before makingTokenList.valuea property, theStripCommentsFilterwas changing the underlying tokens without updating thevalueattribute of the parentTokenList. After makingTokenList.valuea property, those changes did get reflected in the parentTokenList'svalueand as a result the desired line endings were being lost.The most straightforward way I found to address this was to make comment stripping happen before grouping is performed, which required a small amount of hackery to make grouping happen via a filter. I am open to suggestions to better ways to handle this.
I am also a little concerned about the possibility for slowdowns if a particular
TokenList's value is accessed, and thus computed, multiple times, but I didn't actually observe any. This might still be an issue via a codepath I didn't look at. I have some ideas on how to address it if a performance problem comes up, but it would clutter up the code somewhat so I didn't want to implement it unless a need could be demonstrated.