Change _iterate_file_descrs to _get_file_items #10142
Conversation
Returning a materialized list instead of an iterator lets us do simple progress reporting in the future. As the list of FileItems is very lightweight, there shouldn't be a performance hit.
|
Why not just call |
Pierre-Sassoulas
added
the
Needs decision 🔒
Good question. Either way, it kind of ends up being the same thing: we either edit code so that we do |
|
Will check it out, thank you @DanielNoord. |
|
@DanielNoord - this failure actually exists in commit cb12577, immediately prior to my two new commits. I went back a bit just using git reset, and unless there are steps that I'm missing, at appears that this error has been present since at least 713e563 (Thu Oct 5 22:12:56 2023 +0200, Merge branch 'maintenance/3.0.x' into merge-maintenance-into-main): I haven't read the tests, but I'm assuming that each test can be run independently (no global state trash etc). When I run all of the tests as at that commit 713e563, I get a bunch of failures, including that bad test Just for fun, rechecking cb12577 which was the base of my branch: I got few failures than the older commit, but that same test failed (on my machine). |
|
If we change the Here's git blame for that function, it's been around a while: Cheers! |
There was a problem hiding this comment.
As the list of FileItems is very lightweight
I'm not sure this is always true, and I'm not sure it's the main consideration. We need to understand why it was done this way before changing it (the first implementation is not often an iterator for no good reasons).
I don't have the time to read everything to confirm it right now but it seems this is required for multiprocessing, I dug into the git history and here's the MR that introduced this:
Last comment might be interesting: #3016 (comment)
|
Hi @Pierre-Sassoulas , thanks for checking. I was basing this on my review of the code, but that may not have been deep enough. You know, the only reason I suggested this change was to allow for a progress message, “linting 1000 files” eg. Instead of making any changes, I could just make a 2 line function to get the count of items, using mostly the same logic as in the iterator method, which is only called when needed. Would that feel better to you? If yes, this PR can be closed and I can add that in my smaller progress logging draft PR. |
Returning a materialized list instead of an iterator lets us do simple progress reporting in the future. As the list of FileItems is very lightweight, there shouldn't be a performance hit.
Type of Changes
Refs
This work was initially done as part of draft PR #10134 , and it makes sense as a small refactor of the internal method.