Skip to content

feat(api): add support for wiki attachments #2722

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

Merged
merged 2 commits into from
Nov 20, 2023

Conversation

TigreModerata
Copy link
Contributor

@TigreModerata TigreModerata commented Nov 15, 2023

Changes

UploadMixin added, inherits from RESTObject;
UploadMixin used in projects.py and wikis.py to upload files;
Test for upload to wiki api added in tests/functional/api/test_wikis.
Documentation and unit tests not yet added - waiting for corrections/objections

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Merging #2722 (4e7a881) into main (d0546e0) will increase coverage by 0.00%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2722   +/-   ##
=======================================
  Coverage   96.44%   96.45%           
=======================================
  Files          88       88           
  Lines        5761     5775   +14     
=======================================
+ Hits         5556     5570   +14     
  Misses        205      205           
Flag Coverage Δ
api_func_v4 82.44% <87.50%> (+0.04%) ⬆️
cli_func_v4 83.54% <93.75%> (+0.03%) ⬆️
unit 88.12% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
gitlab/mixins.py 92.71% <100.00%> (+0.48%) ⬆️
gitlab/v4/objects/projects.py 98.78% <100.00%> (-0.05%) ⬇️
gitlab/v4/objects/wikis.py 100.00% <100.00%> (ø)

Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

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

Thanks a lot @TigreModerata, I like the direction here but just had a question about the exception raising. This would also potentially address the coverage drop I think. Let me know what you think!

@TigreModerata
Copy link
Contributor Author

I removed the exception. Changed _upload_path to _upload_string in Project, ProjectWiki and UploadMixin.
I keep messing up the commit message it seems...

If you are happy with the overall approach, maybe I should add the tests for the mixin and other unit tests (and update the docs)?

@TigreModerata
Copy link
Contributor Author

Sorry again about the commit messages, I'll fix it in the next one, have to run!

@nejch
Copy link
Member

nejch commented Nov 17, 2023

I removed the exception. Changed _upload_path to _upload_string in Project, ProjectWiki and UploadMixin.
I keep messing up the commit message it seems...

If you are happy with the overall approach, maybe I should add the tests for the mixin and other unit tests (and update the docs)?

My suggestion was actually in the other direction (to use _upload_path everywhere), as that's consistent with our _path and _computed_path attributes. Otherwise I agree, we can go ahead with this and update the tests/docs, yes! Thanks so much!

@nejch
Copy link
Member

nejch commented Nov 17, 2023

Sorry again about the commit messages, I'll fix it in the next one, have to run!

@TigreModerata no worries, it's best to just squash the commits into one when you're finished with the fixups.

@TigreModerata
Copy link
Contributor Author

TigreModerata commented Nov 19, 2023

OK, I managed an ok commit :)
I added a section to the wikis documentation about the file upload and a unit test for the UploadMixin.
Also added upload support for GroupWiki, api tests for it and docs section.

Added UploadMixin in mixin module
Added UplaodMixin dependency for Project, ProjectWiki, GroupWiki
Added api tests for wiki upload
Added unit test for mixin
Added docs sections to wikis.rst
Copy link
Member

@nejch nejch left a comment

Choose a reason for hiding this comment

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

Thanks again @TigreModerata, I had a few more comments here, let me know what you think!

Added UploadMixin in mixin module
Added UplaodMixin dependency for Project, ProjectWiki, GroupWiki
Added api tests for wiki upload
Added unit test for mixin
Added docs sections to wikis.rst
@TigreModerata
Copy link
Contributor Author

Separated unit tests for upload mixin and re-added the file upload unit tests for projects

@nejch nejch merged commit 7b864b8 into python-gitlab:main Nov 20, 2023
@nejch
Copy link
Member

nejch commented Nov 20, 2023

Thanks a lot for your patience @TigreModerata! I squashed the commits in the end as there were 2 identical commit messages 🙇

@TigreModerata
Copy link
Contributor Author

Thanks a lot for your patience @TigreModerata! I squashed the commits in the end as there were 2 identical commit messages 🙇

OK, l have no idea what happened (I did many wrong resets, to squash commits, and I never hit the right point it seems) - I will learn one day. Thank YOU for your patience, are you kidding! I do realize that my "help" is more of a chore, but you guys' help and patience is the only way I make any progress at all!
Speaking of, if there are any other old issues that would be useful to tackle, I'm always happy to try (and commit properly maybe).

@TigreModerata TigreModerata deleted the WikiUploadFile branch November 20, 2023 16:18
@nejch
Copy link
Member

nejch commented Nov 20, 2023

No worries @TigreModerata reviewing can be easier than starting things sometimes 😅

Most open issues could use some more love but some are definitely lower-hanging fruit - I'd say things that are labelled feature are the most similar to what you've done here, i.e. if it's about implementing endpoints that we're missing here:

https://github.com/python-gitlab/python-gitlab/issues?q=is%3Aissue+is%3Aopen+label%3Afeature

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.

2 participants