Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change refspec api #290

Merged
merged 5 commits into from
Dec 2, 2013
Merged

Change refspec api #290

merged 5 commits into from
Dec 2, 2013

Conversation

jplana
Copy link
Contributor

@jplana jplana commented Nov 25, 2013

New attempt to provide refspec API with pygit, now using the new functions in libgit 0.20

It will expose:

the attributes

  • fetch_refspecs
  • push_refspecs

returning a list of strings with the refspecs

the methods

  • set_fetch_refspecs([str])
  • set_push_refspecs([str])

setting a list of strings a new refspecs

PyList_SET_ITEM(
new_list,
index,
PyString_FromString(strarray->strings[index]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to_unicode for this which works across verisions.

@jdavid
Copy link
Member

jdavid commented Nov 25, 2013

I think the API will be more intuitive if using a getset instead of a method, so:

refspecs = remote.fetch_refspecs
remote.fetch_refspecs = refspecs

Please update the documentation: docs/remotes.rst

Thanks!

@jplana
Copy link
Contributor Author

jplana commented Nov 26, 2013

I fixed most of the issues here, except probably the most important, the API. I see one problem with your proposal, while this would work:

remote.fetch_refspecs = ['xxx:xxxx','yyyy:zzzz']

This would not produce the expected result:

remote.fetch_refspecs[0] = 'xxxx:xxxx'

which a user of the library might use, as this is correct:

print remote.fetch_refspecs[0]
>>> 'xxxx:xxxx' 

IMHO a list where you could do

remote.fetch_refspecs[0] = 'xxxx:xxx'
# or even
remote.fetch_refspec.append('xxxx:xxxx')
remove.fetch_refspec.remove('yyyy:yyyy')

would be nicer, though I would leave it for a future PR (comments on this are welcome)

BTW I don't know if the attribute get_refspec makes sense anymore as the user can just "iterate" all refspecs using the refspec_count total but won't know if it's either a push or fetch refspec, not very useful.

@jdavid
Copy link
Member

jdavid commented Nov 26, 2013

Then it should be Remote.get_fetch_refspecs() so the API is symmetric.

@jplana
Copy link
Contributor Author

jplana commented Dec 2, 2013

I think that fixes all the comments, adding better error treatment and making the API symmetric.

@jdavid
Copy link
Member

jdavid commented Dec 2, 2013

After merging I get this error:

$ python setup.py build
[...]
x86_64-pc-linux-gnu-gcc -pthread -fPIC -I/usr/local/include -Iinclude -I/usr/include/python2.7 -c src/remote.c -o build/temp.linux-x86_64-2.7/src/remote.o
src/remote.c: In function ‘Remote_set_fetch_refspecs’:
src/remote.c:503:1: error: expected declaration or statement at end of input
error: command 'x86_64-pc-linux-gnu-gcc' failed with exit status 1

@jplana
Copy link
Contributor Author

jplana commented Dec 2, 2013

Upssss. Sorry about it.

@jdavid jdavid merged commit 6050ae0 into libgit2:master Dec 2, 2013
@jplana
Copy link
Contributor Author

jplana commented Dec 2, 2013

Thanks a lot for your patience :-D

@jdavid
Copy link
Member

jdavid commented Dec 2, 2013

Thank you for contributing.

Note I have fixed error handling. The Python functions already set an exception whenever there is an error, so you only need to set the exception for anything else.

@jplana
Copy link
Contributor Author

jplana commented Dec 3, 2013

I see the changes, thanks a lot. Still learning how to do proper python exceptions :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants