thecodrr
alternative iterators API
an alternative to #154
draft until one day we can settle on a direction. probably end up with a 3rd branch once this one has been reviewed
I was able to get a prototype working with full iterator support using opendir. Will open a PR soon for testing and review. It's still a little rough but looks "okay" overall.
this is a "full iterator", switching it to use opendir is trivial now since its just an implementation detail
do you mean you made a new branch unrelated to this and my other one?
if that's the case, feedback would have been nice as these two branches took a lot of effort. and every comment that's been made was stuff i was already well aware of, but didn't want scope creep. i just needed to know we were open to larger change and would have done it
i just wish you said sooner so i didn't spend so much time building this 😅
do you mean you made a new branch unrelated to this and my other one?
Not completely unrelated since the whole thing is still based on your PR (and idea). I was experimenting yesterday with your PR trying different approaches and came upon it. Still not sure how it performs or even if it is merge worthy yet.
i just wish you said sooner so i didn't spend so much time building this 😅
I am sorry if the above comment came across as me undermining your work. My intention was to share a different approach to the same problem and see if we can reach a better solution overall. Especially since its a fundamentally different approach, I couldn't just leave a review.
I hope that clarifies things.
No worries at least this is still somewhat useful to help us figure out how we want to implement this
To be honest, I think we need to rework the various push functions. Split them into test functions ("should we push this"), and push functions (actually push it).
Then the iterator can share the same tests but do its own pushing (yielding).
If we switch to opendir, we won't need any of the push functions but will need the test functions (to avoid duplicating conditional logic)
@43081j that is certainly a design limitation right now. The main idea behind this approach was to reduce branching on the hot paths e.g. if you don't use a filter, the push function should be inlined by the engine. Is there a way to separate the "check" part and the "push" part without introducing branching at the callsite?
Maybe we can have a "file filter" and a "directory filter" we build up at initialisation time, the same way we do with push functions now
Then any time we want to push something, we call the appropriate filter and just directly push (or have a separate push function)
So basically you would fileFilter.build(options) or something. Which checks if there's filters and returns a static truthy function if there aren't
I see no way to yield without at least if condition. Even if we build a filter on init, we'll have to check its result inside the loop. The only other alternative I found was to build a generator version of pushFile and pushDirectory and then just yield *. Unfortunately, that is very slow compared to just doing if then yield.
yes we would need a condition either way, like this:
if (this.filterFileFn(path)) {
yield path;
}
where:
filterFileFn = filterFile.build(options, state);
// and elsewhere
const allFilter = () => true;
function build(options, state) {
if (!options.filters) {
return allFilter;
}
// etc...
}
is your concern that the non-iterator code can bypass this because it won't have a condition? it'll just always push(path) and rely on the fact the push function does no conditional logic (if no filters were set)?
there's not really a clean way to achieve that when yielding i think. but a no-op function that is always true should be pretty fast
I was able to get a prototype working with full iterator support using
opendir. Will open a PR soon for testing and review. It's still a little rough but looks "okay" overall.
any updates on this? would love to test it out and give feedback
