feat(api): add group hooks#1533
feat(api): add group hooks#1533nejch merged 2 commits intopython-gitlab:masterfrom sugonyak:add-group-hooks
Conversation
|
Got a few questions:
|
Codecov Report
@@ Coverage Diff @@
## master #1533 +/- ##
==========================================
+ Coverage 91.13% 91.15% +0.02%
==========================================
Files 74 74
Lines 4162 4172 +10
==========================================
+ Hits 3793 3803 +10
Misses 369 369
Flags with carried forward coverage won't be shown. Click here to find out more.
|
nejch
left a comment
nejch
left a comment
There was a problem hiding this comment.
Thanks a lot @sugonyak! Looks good. Just a few comments and to answer your questions:
Not quite sure if I should change something in CLI part, could someone advise?
Since you only use standard methods, the CLI part should be auto-generated including docs and nothing extra is needed here.
Also, tried to use pre-commit checks and mypy gives me a lot of errors in files not affected by my changes, is it normal?
Sorry about that, looks like the mypy pre-commit does not respect the file list from mypy's config, I need to check. If tox -e mypy is happy then for now just bypass it, I'll try to fix it asap.
I see that tests for project hooks are not implemented, should I implement some for group hooks?
I've suggested factoring them out into a common file, then it should be easier to add tests and maybe reuse fixtures. But even if you just add group tests that's fine, just maybe really put them into test_hooks.py for easier reuse. Thanks!
tests/unit/objects/test_groups.py
Outdated
|
|
||
|
|
||
| @pytest.mark.skip(reason="missing test") | ||
| def test_list_group_hooks(gl): | ||
| pass | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="missing test") | ||
| def test_get_group_hook(gl): | ||
| pass | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="missing test") | ||
| def test_create_group_hook(gl): | ||
| pass | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="missing test") | ||
| def test_update_group_hook(gl): | ||
| pass | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason="missing test") | ||
| def test_delete_group_hook(gl): | ||
| pass |
There was a problem hiding this comment.
Perhaps we could factor this (and project hooks from test_projects.py) out into tests/unit/objects/test_hooks.py and they could share most of the fixture/setup?
See for example the issues statistics test that shares between project/group/instance endpoints:
https://github.com/python-gitlab/python-gitlab/blob/master/tests/unit/objects/test_issues.py#L44-L58
There was a problem hiding this comment.
Yeah, seems like a good idea.
nejch
left a comment
nejch
left a comment
There was a problem hiding this comment.
Awesome, thanks for adding all the unit tests even for other endpoints.
We'll just need to skip the functional test for now unfortunately as we only run them against the gitlab-ce container in CI.
3 participants
Resolves #1496