[WIP] Refactor & added cycle option

[oerpli]

Here's a first PR with my progress so far. I splitted up the different parts in seperate commits (making diffs easier to handle) - if you disagree with anything, let me know and I will rework it.

In chronological order:

• Formatted with black: A reformat of the whole project - to keep other diffs independent of any white-space or similar noise (a lot of lines touched but in most cases only replacing ' with ".
• Added 'cycle' option: This was added in fzf 0.18.0 I think and is what I missed most about iterfzf so far. If you want this in a seperate PR or not at all I will throw it out.
• Refactor: The "big one". Explanation below
• Cycle non-default: I personally think that cycle should be on by default. Others might disagree, so I included this commit (but as the last one to make reverting easy).

Refactoring: My main goal was to make following the logic of the function a bit easier. It is now seperated as follows:

1. iterfzf: Call with whatever arguments are translated into corresponding fzf call which is then executed with _exec_cmd and its result are evaluated based on user-preference.
2. _exec_cmd: I was somewhat confused by the initial design with all the = None declarations which where then assigned properly on the first loop-iteration. I assume that this was related to the fact that the input might be an iterable with huge delays on even the first item as well as possibly no items. I changed it s.t. I first get the first element of the iterable via iterator, initialize all the things and then put the first element and the rest of the iterable back together via itertools.chain([first],remaining) and loop through the elements. This design makes sure that iteration still only happens once (and element-by-element) and each element is handled exactly the same way (branching only to capture exceptions and handle is_byte cases.

Possible concerns:

Is None a valid object in the input-iterable?

first = next(iterator, None)
if not first:
     [ ... early exit...]

Maybe this is a problem because if only the first element of the iterable is None this would result in an early exit despite the iterator not being empty. I found some information here. I am not sure whether this is a problem, as iterfzf insists on input being bytes or str and None is at least not a central example of those (ymmv). If this is a problem, the fix would be to replace first = next(iterator, None) with something like first = next(iterator, unique_object) and then test for identity with that unique object.

print_query

I found the handling of the fzf-result was kind of convoluted due to print_query and I didn't exactly see what its purpose was. I changed it to the following:

1. Always pass that options to fzf (i.e. to _exec_cmd)
2. Only return it back to the caller of iterfzf if it was specified.

Basically, the result for the user should be identical, but the code is cleaner.

Other than that I didn't stray very far from your initial implementations, mostly reworked some nested branchings.

I am looking forward to any input you might have!

PS: I will investigate the crash-issue (#9) when I find some time on one of the next evenings.

[oerpli]

I used tuple unpacking initially (which seems to be not supported by Python 2.7. I added another commit to remove it as well as fix the case with the test case with the empty iterable.

From what I've glanced the first CI job still fails but the only error I found was due to pylint (max line length in setup.py).

I find 90 easier to read than 80 but if you want to stay with 80 I will update my PR tomorrow.

[oerpli]

I addressed two small things you mentioned in the review. I will address the rest (and undo the formatting changes) in the next days.

[oerpli]

I had a few unexpected spare minutes and addressed your concerns except:

• Didn't split up into multiple PRs
• type(iterfzf(['',''])) == NoneType: As far as I know, this was already the case before my PR. Generally, there are some weird edge cases, e.g.:

  iterfzf(['', ''], multi=True) == None      # if selecting zero or one elements
  iterfzf(['', ''], multi=True) == [ '', '' ]# if selecting both 

Though I would leave this for a seperate issue as I am not sure, if this is desired behavior.

I also added it to the Changelog. I assumed that it would be released as 0.7.0.20.0.

[oerpli]

The CI builds seem to fail but I don't really see what's the problem. From the time it took to run them it seems like some timeout (waiting for input maybe?). Any ideas?

[dahlia]

Sorry for my late response.

Though I would leave this for a seperate issue as I am not sure, if this is desired behavior.

It's a bug, so we should fix it. 🙄

The CI builds seem to fail but I don't really see what's the problem. From the time it took to run them it seems like some timeout (waiting for input maybe?). Any ideas?

I'm sorry. I told you “The existing behavior on an empty iterable had not been early return…”, it was wrong. The existing behavior on empty iterable had been early return with None (or (None, None) when print_query=True), and there is even test code in tox.ini for exactly that behavior:

python -c "import sys; sys.path.remove(''); from iterfzf import iterfzf; assert iterfzf([]) is None"
python -c "import sys; sys.path.remove(''); from iterfzf import iterfzf; assert iterfzf([], multi=True) is None"

The reason your patch does not pass the CI is that the above test actually invokes fzf so that it waits a user's input through tty…

Although I still this behavior is better to change, but in this patch, it seems better to stick with the existing behavior. Sorry for my misguiding. 🙏

[oerpli]

I think it should be good now - though the one CI build still fails. If you happen to know why that is, I can fix it tomorrow.