Skip to content
This repository was archived by the owner on Nov 4, 2020. It is now read-only.

Change to projecthook calls to use kwargs allowing for newer params#181

Open
owmtia wants to merge 5 commits intopyapi-gitlab:developfrom
owmtia:develop
Open

Change to projecthook calls to use kwargs allowing for newer params#181
owmtia wants to merge 5 commits intopyapi-gitlab:developfrom
owmtia:develop

Conversation

@owmtia
Copy link
Copy Markdown
Contributor

@owmtia owmtia commented Apr 19, 2016

I particular needed access to enable_ssl_verification param but changing the code to use kwargs seemed to be more consistent with other sections of code.

data['merge_requests_events'] = int(bool(merge_requests))
data['tag_push_events'] = int(bool(tag_push))
if kwargs:
data.update(kwargs)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doesn't this change the data sent over the wire from "0/1" (int) to "False/True"?

@owmtia
Copy link
Copy Markdown
Contributor Author

owmtia commented Apr 20, 2016

I have added the code to change the values to 0/1, I have assumed everything is a bool which seems to be case at the moment with the api. I also restored the previous param as I released it would break backwards compatibility especially as the params have different names to the gitlab API params. I also added the extra documentation to the function.

@FredrikWendt
Copy link
Copy Markdown

Great.
My only concern is this: if a user of this python lib wants to specify a non-boolean typed argument that is visible and documented in the GitLab API, would they know that the value would be converted to 0 or 1.

>>> int(bool("0"))
1
>>> int(bool("False"))
1
>>> int(bool("True"))
1
>>> int(bool("true"))
1
>>> int(bool(True))
1
>>> int(bool(False))
0
>>> int(bool("AnyString"))
1

I'm not sure we need to care right now.

@owmtia
Copy link
Copy Markdown
Contributor Author

owmtia commented Apr 20, 2016

Yes, I had thought about putting in an isinstance to check for bools/ints but it didn't seem necessary with the current api being all bools.

@ehelms
Copy link
Copy Markdown

ehelms commented Apr 20, 2016

👍

@drzraf
Copy link
Copy Markdown

drzraf commented May 5, 2018

  1. "note","job","pipeline","wiki_page" and "confidential_issue" are missing
  2. "token" is missing and would not be a boolean argument but rather a string.

See: https://docs.gitlab.com/ce/api/projects.html#list-project-hooks

+1 for improving this API endpoint

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants