[feat] Forgejo issues and comment / reactions #959
Conversation
Summary of ChangesHello @mfocko, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces robust support for Forgejo's issue and comment functionalities within the ogr library. It provides a complete set of tools for interacting with Forgejo issues, including their creation, retrieval, modification, and lifecycle management, as well as handling comments and reactions for both issues and pull requests. These enhancements aim to integrate Forgejo more deeply into the ogr ecosystem, offering a consistent interface for managing project discussions and tasks. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for Forgejo issues, comments, and reactions. The implementation looks good overall, but there are a few issues to address. I've found a critical issue in the test setup that would prevent tests from running, a couple of high-severity bugs in the comments and pull_request modules that would lead to runtime errors or incorrect behavior, and several medium-severity issues related to code clarity, redundancy, and exception handling. Please see my detailed comments for suggestions on how to fix them.
ogr/services/forgejo/comments.py
Outdated
| def _get_owner_and_repo(self): | ||
| issue_url = self._raw_comment.issue_url | ||
| parts = urlparse(issue_url).path.strip("/").split("/") | ||
| namespace, repo = parts[0], parts[1] | ||
| return (namespace, repo) |
There was a problem hiding this comment.
This method _get_owner_and_repo is not used anywhere in the codebase. It should be removed to avoid having dead code.
| project: "forgejo.ForgejoProject", | ||
| ): | ||
| super().__init__(raw_pr, project) | ||
| self.project = project |
There was a problem hiding this comment.
The assignment self.project = project is redundant. The parent class BasePullRequest already initializes self._target_project with the project argument, and provides a project property to access it.
There was a problem hiding this comment.
I would leave this be the way it is.
ogr/services/forgejo/pull_request.py
Outdated
| ) | ||
| ) | ||
| except NotFoundError as ex: | ||
| raise NotFoundError(f"{ex}") from ex |
There was a problem hiding this comment.
Re-raising NotFoundError with f"{ex}" as the message is redundant, as it just uses the original exception's message. You can simply re-raise the exception to preserve the original traceback and message.
| raise NotFoundError(f"{ex}") from ex | |
| raise |
|
Build failed. ✔️ pre-commit SUCCESS in 2m 59s |
ogr/services/forgejo/issue.py
Outdated
| repo=project.repo, | ||
| title=title, | ||
| body=body, | ||
| labels=labels, |
There was a problem hiding this comment.
The labels parameter of the create_issue method is of the Optional[Sequence[int]] type, but the variable that is passed here is of the type Optional[list[str]]. Some sort of conversion is required here.
The docstring of the create_issue method:
labels : typing.Optional[typing.Sequence[int]]
list of label ids
|
Hi @mynk8, I'm following up on your progress on these Forgejo-related features. Is this something you still plan to work on, or would you like us to step in and finish the rest? |
|
Hi, thanks for the follow-up. Please feel free to continue and complete the remaining work. I’m currently focused on other things and won’t be able to take this forward right now. |
Thanks for the quick response. I'll try to finish the remaining parts of this PR, then. Thank you again for your contribution. 🙏 |
|
✔️ pre-commit SUCCESS in 2m 53s |
betulependule
force-pushed
the
feat/forgejo/issues
branch
from
5c3e840 to
f7ccdf9
Compare
|
✔️ pre-commit SUCCESS in 2m 49s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 22s |
|
❌ pre-commit FAILURE in 2m 45s |
betulependule
force-pushed
the
feat/forgejo/issues
branch
from
5cc2daf to
8961a0a
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 43s |
betulependule
force-pushed
the
feat/forgejo/issues
branch
from
8961a0a to
bbcb2cf
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 2m 49s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 16s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 05s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 17s |
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 18s |
betulependule
force-pushed
the
feat/forgejo/issues
branch
from
110cca8 to
c43b10b
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 19s |
It is now possible to delete reactions on comments. During testing, it has been discovered that whenever the get_reactions method is called on a comment with no reactions, pyforgejo raises ApiError. It has also been discovered that re-recording tests results in ValidationError from pydantic, though this error isn't raised again when re-running tests. Further investigation is required.
Previously, the title and description setters called the API properly when editing the title / description. However, the returned raw issue was discarded, which is no longer the case.
The labels parameter is a list of strings. However, Forgejo API expects a list of label ids. The implementation of create_issue has been edited so that when the labels parameter is specified, the ids of these labels are retrieved from the API. If a label, that doesn't yet exist, is specified, then a new label is created via the API and its id is used when creating the issue.
The partial_api would previously add the issue number parameter when making an API call based on what method was being used. This part of implementation has been removed to make the code easier to understand.
Previously, trying to retrieve issues of a nonexistent author, an empty list was returned. Raising an exception instead makes it more clear that something went wrong.
It is not necessary to check whether the assignee object has the login attribute.
Previously when adding an assignee would not update the raw issue stored inside the Issue object. A test has been added to cover this fix.
This is so that the exception is the same as when trying the same via the Github API.
The __update_info method was brought back for this. The add_label method defined by pyforgejo's API doesn't return the updated issue, so it has to be retrieved separately.
Previously, the list of existing labels had to be retrieved from the API so that ids of labels to be added could be determined. When adding a non-existent label, it had to be created by a separate API call as well, leading to somewhat hard-to-understand code. This code has been removed and instead, an issue is always created without any labels and then the add_label is called instead, leading to two more API calls being made whenever labels are to be added to the issue (regardless of the amount of labels).
betulependule
force-pushed
the
feat/forgejo/issues
branch
from
d574b54 to
d7a4c3e
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 3m 11s |
TODO:
packit/packit.dev.Fixes #880
Fixes #883
Related to #866
Merge before/after
RELEASE NOTES BEGIN
Ogr now supports Forgejo issues, comments and comment reactions.
RELEASE NOTES END