Move checkout to CFFI and add a target directory option#390
Move checkout to CFFI and add a target directory option#390jdavid merged 2 commits intolibgit2:masterfrom
Conversation
pygit2/repository.py
Outdated
There was a problem hiding this comment.
Why do you need to iterate? You can do it with 2 if statements:
if 'strategy' in kwargs:
copts.checkout_strategy = kwargs['strategy']
if 'directory' in kwargs:
target_dir = ffi.new('char[]', to_str(kwargs['directory']))
refs.append(target_dir)
copts.target_directory = target_dirThere was a problem hiding this comment.
It's a leftover from working with less abstract constructions; but it's a guess as to how many parameters will actually be passed versus how many we will be looking at. I'm not a fan of looking up the same key twice, but it can be reduced.
There was a problem hiding this comment.
I remember now why I wanted to iterate over the inputs instead of checking for the known keys: if the programmer using this method makes a typo, we will ignore that option instead of complaining, which can cause subtle bugs, as there is no indication that we're ignoring an argument that was passed. I didn't end up doing it as we just have the two things, but when this gets extended, we should raise an exception if we get something we don't recognise as an option.
There was a problem hiding this comment.
Just changed this static method so it is shorter and fails on unexpected arguments.
As part of this, make the strategy part of **kwargs, in preparation for supporting more options.
Having it at the python level should also make it easer to extend with more options in the future as needed.