Start simplifying collection code in Session #7670
Conversation
|
This is awesome work @bluetech! Have been meaning to do what you did here for some time now, glad you took this head on. It is a pleasure reading commit by commit, thanks for that. I find it specially beautiful that in the end it was possible to get rid of the instance level caches into local variables, as they should. Again, great work! 👍 |
|
Btw we probably should squash/merge this once reviewed, I think? |
|
@nicoddemus for refactoring chains like this i believe its good to actually keep them in a chain of commits , as that keeps history more comprehensible for curated history im against squashing |
Good point, agree. 👍 |
It doesn't add much, mostly just an eye sore, particularly with the overloads.
Similar to the previous commit, this makes things more straightforward.
This reverts commit f10ab02. The commit was good in that it removed a non-trivial amount of code duplication. However it was done in the wrong layer (nodes.py) and split up a major part of the collection (the filesystem traversal) to a separate class making it harder to understand. We should try to reduce the duplication, but in a more appropriate manner.
The end result in the `else` branch is the same, but flows naturally.
Already covered in a condition above.
This is a more sensible interface for matchnodes. This also fixes a sort-of bug where a recursive call to matchnodes raises NoMatch which would terminate the entire tree, even if other branches may find a match. Though I don't think it's actually possible.
It's a little more sane this way.
The weird name was due to f396733, now that I understand it a bit better can give it a more descriptive name.
Things are easier to understand without the weird exception.
This removes an unhelpful level of indirection and enables some upcoming upcoming simplifications.
Now all of the logic is in one place and may be simplified and refactored in more sensible way.
They are only used for the duration of this function.
bluetech
force-pushed
the
session-inline
branch
from
7ecdeac to
c1f9756
Compare
|
Thanks everyone who looked at all of these commits :) |
Collection starts in
Session.perform_collect(). I am trying to understand what it does, but it's very complex (in the cyclomatic complexity sense) and hard to follow. This PR starts to try and reveal the inner beauty of the code :)The PR is split into commits, each doing one refactoring. Most of them are straightforward (except for
Revert "Move common code between Session and Package to FSCollector"perhaps) and should be easier to follow than reading the full diff.The end result is that most of the logic is localized in the
Session.collect()instead of being spread around many places. The next step would be to figure out what all of the caches and special cases do, and either simplify and document them, or (I suspect) plain remove them if they're not serving a purpose.