implement support for Forgejo comments and reactions #906

julietowah · 2025-03-24T11:15:55Z

Implement Support for Forgejo Comments and Reactions

Description:
This PR introduces support for handling comments and reactions within Forgejo. The changes include implementing classes for managing issue comments, pull request comments, commit comments, and their associated reactions. The implementation follows the structure of the existing ogr framework while integrating Forgejo API functionalities.

Changes in this PR:

Created the comments.py file in ogr/services/forgejo
Added ForgejoReaction class
- Supports adding and deleting reactions to comments.
- Retrieves reactions for a given comment.
- Extracts user login and content from Forgejo's API response.
Added ForgejoComment class
- Allows retrieving, editing, and updating comments.
- Supports fetching associated reactions.
- Provides structured access to comment metadata (author, timestamps, etc.).
Added Specialized Comment Classes:
- ForgejoIssueComment: Represents comments on issues.
- ForgejoPRComment: Represents comments on pull requests.
- ForgejoCommitComment: Represents comments on commits.

fixes #883

softwarefactory-project-zuul · 2025-03-24T11:34:16Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/0267cc95560541ecb38b9fd3889c70ae

✔️ pre-commit SUCCESS in 2m 34s
✔️ ogr-tests-pip-deps SUCCESS in 17m 29s

julietowah · 2025-03-25T08:42:03Z

Please @morucci @lbarcziova i tried to add mergeit label but it seems i don't have the necessary permission.

LecrisUT

The original issue is about implementation for forgejo please implement it there first ¹. The generalisation in the abstract is a more difficult task that requires some refactoring.

https://github.com/packit/ogr/tree/main/ogr/services/forgejo ↩

LecrisUT · 2025-03-26T08:15:39Z

recipe.yaml

@@ -27,6 +27,7 @@
        name:
          - twine # we need newest twine, b/c of the check command
          - readme_renderer[md]
+          - pytest-cov


Could you open a separate PR for this? I think it would be better to drop the default --cov flags and create a separate test environment that would integrate with codecov or such.

You might want to rebase onto main. This change has been incorporated in #893 (learning opportunity to get some intuition of what git rebase does and how to handle its conflicts 🙂)

LecrisUT · 2025-03-26T08:20:39Z

ogr/abstract.py


    def __str__(self):
        return f"Reaction(raw_reaction={self._raw_reaction})"

    def delete(self) -> None:
        """Delete a reaction."""
-        raise NotImplementedError()
+        if not self._id:
+            raise ValueError("Cannot delete a reaction without an ID.")


This clashes with reaction_id being optional. If the intent is that id is always present then there should be an overwritable @property with the default being to read self._id or raise.

LecrisUT · 2025-03-26T08:22:13Z

ogr/abstract.py

+        raw_reaction: Any,
+        reaction_id: Optional[int] = None,
+        parent: Optional[Any] = None,
+    ) -> None:


Documentation would be appreciated. No idea what parent is meant to be.

LecrisUT · 2025-03-26T08:26:53Z

ogr/abstract.py

+        self._body = raw_comment["body"]
+        self._author = raw_comment["user"]["login"]
+        self._created = raw_comment["created_at"]
+        self._edited = raw_comment.get("updated_at")


Hmm, this is a strong assumption. Either make the type-hinting of raw_comment more explicit or let it be overwritten in the actual implementation. Same for the other parts.

Ok thank you am on it

Just to double check. I am not recommending to tackle type-hinting approach in this PR (maybe in a separate one). Instead you should try to implement it in the /ogr/services/forgejo files first.

Please also drop the changes made in abstract.py. I saw you dropped some with a33e90e, but the remaining changes should also be dropped for now.

softwarefactory-project-zuul · 2025-04-01T10:19:30Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/7828d0c425ab4760803573adf8ddfd9e

✔️ pre-commit SUCCESS in 2m 40s
✔️ ogr-tests-pip-deps SUCCESS in 17m 44s

softwarefactory-project-zuul · 2025-04-01T15:00:01Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/747f6c4a2bae4bb19524827136d042e4

✔️ pre-commit SUCCESS in 2m 36s
✔️ ogr-tests-pip-deps SUCCESS in 17m 23s

lbarcziova

thanks a lot for having a look into this!

To clarify the code structure, the ogr/abstract.py should not be changed anyhow, it just holds the interface that is then implemented for particular forges in respective directories in ogr/services (e.g. github). So can you please move the implementation here, creating new files? You can check how it is done for other forges e.g. https://github.com/packit/ogr/tree/main/ogr/services/github and also having a look into implementation in ogr/services/forgejo can be helpful. Let me know if this is clear!

lbarcziova · 2025-04-02T09:17:02Z

ogr/abstract.py

-    ex: Union[
-        github.GithubException,
-        gitlab.GitlabError,
-        pyforgejo.core.api_error.ApiError,
-    ],


I believe these changes related to exceptions were not intended, or?

Apologies, for the unintended change. I'll work on them and push an update. Please hold off on reviewing until I fix it. Thanks!

i think this came from when i pull recent changes from the before i pushed to the pr

julietowah · 2025-04-02T16:28:08Z

#906 (review)

Thank you so much,

You mean i should create a file and impliment the logic at https://github.com/packit/ogr/tree/main/ogr/services/forgejo following the examples at https://github.com/packit/ogr/tree/main/ogr/services/github

julietowah · 2025-04-03T14:16:30Z

Hello @lbarcziova i want to create a pr for this implementation but i want to use a new pr altogether.
will it be okay?

LecrisUT · 2025-04-03T14:32:02Z

Generally it is preferred to rebase and continue in the same PR in order to keep the discussion history, but if you feel like you need a fresh start at it, then you can restart it, as long as you do not do it excessively that it would confuse people.

To me the discussions so far have been quite minimal and I wouldn't do a fresh start yet (reviews can easily get to ~100 comments and I still go back to those comments every now and then). Instead, you can mark conversations as resolved, and I sometimes mark my own comments when I feel it is outdated.

julietowah · 2025-04-03T14:35:38Z

Ok thank you

softwarefactory-project-zuul · 2025-04-03T16:33:52Z

Build failed.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/2ed9b7542689409f9db9691632545268

✔️ pre-commit SUCCESS in 3m 41s
❌ ogr-tests-pip-deps FAILURE in 17m 21s

softwarefactory-project-zuul · 2025-04-05T14:21:15Z

Build succeeded.
https://softwarefactory-project.io/zuul/t/packit-service/buildset/327c06c04556481e889256aa77461fed

✔️ pre-commit SUCCESS in 3m 48s
✔️ ogr-tests-pip-deps SUCCESS in 17m 24s

julietowah · 2025-04-06T06:12:45Z

Hello ,
I think its all good now

LecrisUT

I would suggest you either try:

work locally similar to the github example in the readme, but instead use codeberg and for example try to read the reactions on this issue. Or better yet, open up a test repo for you to play with and try the whole read+write api (be a good netizen and don't test write on other people's public repos)
or see the test-suite and reimplement the github comments tests in the form of the forgejo tests

LecrisUT · 2025-04-07T06:00:54Z

ogr/services/forgejo/comments.py

+        forgejo_client: Any,
+        repository: str,
+        issue_number: int,
+    ) -> None:


Where does this __init__ come from. I don't see a reference to forgejo_client in the current codebase

julietowah · 2025-04-07T09:30:34Z

I would suggest you either try:

work locally similar to the github example in the readme, but instead use codeberg and for example try to read the reactions on this issue. Or better yet, open up a test repo for you to play with and try the whole read+write api (be a good netizen and don't test write on other people's public repos)

or see the test-suite and reimplement the github comments tests in the form of the forgejo tests

Ok let me work on it

lbarcziova

Having a closer look at the PR, this functionality unfortunately depends a bit on the implementation of the ForgejoPullRequest and ForgejoIssue class. To be able to actually access the comments (=> and reactions), also the get_comment and _get_all_comments need to be implemented for both of them, see example in Gitlab implementation here and here.

lbarcziova · 2025-04-07T14:19:49Z

ogr/services/forgejo/comments.py

+        url = (
+            f"{self._forgejo_client.forgejo_url}/api/v1/repos/"
+            f"{self._repository}/issues/{self._issue_number}/reactions/{self._id}"
+        )
+        self._forgejo_client._make_request("DELETE", url)


we are using the client library methods, see examples here, that file can be an inspiration on how to implement the methods. So for this particular case, you would use this method. And similarly, for other methods where you currently construct the URL manually.

Thank you so much for this clarity

I am working on implementing ForgejoIssue (#896) and it also depends on ForgejoIssueComments, I can collaborate to complete this PR.

Thank you @mynk8 will you have time today?

Wouldn't the dependency be the other way around? Have this PR depend on #896? One simple approach is to rebase this on top of #896 and update it as the PR evolves. This is also a good example why you should have a concise git history

It's kind of true, I need ForgejoIssueComments for get_comments and _get_all_comments and this is where I think I can help with here.

Oh yeah, I missed that this PR is now implementing comments in general. Up to you how it is easier to coordinate. You could also make it a NotImplementedError and follow it up in a subsequent PR. No need to implement everything in a single PR, as long as the minimal functionality can be tested

julietowah marked this pull request as draft March 25, 2025 12:55

julietowah marked this pull request as ready for review March 25, 2025 13:01

julietowah mentioned this pull request Mar 26, 2025

Implement support for Forgejo comments and reactions on it #883

Open

2 tasks

LecrisUT suggested changes Mar 26, 2025

View reviewed changes

julietowah changed the title ~~implement methods for the classes inheriting from Comment and Reaction https://github.com/packit/ogr/issues/883~~ implement methods for the classes inheriting from Comment and Reaction Mar 27, 2025

julietowah changed the title ~~implement methods for the classes inheriting from Comment and Reaction~~ implement support for Forgejo comments and reactions Mar 27, 2025

julietowah force-pushed the feature/implement-comment-reaction branch from e7f684e to 93d476d Compare April 1, 2025 14:42

lbarcziova requested changes Apr 2, 2025

View reviewed changes

update: implement support for Forgejo comments and reactions

381972a

julietowah force-pushed the feature/implement-comment-reaction branch from afa6147 to 381972a Compare April 5, 2025 14:03

LecrisUT suggested changes Apr 7, 2025

View reviewed changes

lbarcziova reviewed Apr 7, 2025

View reviewed changes

implement support for Forgejo comments and reactions #906

Are you sure you want to change the base?

implement support for Forgejo comments and reactions #906

Uh oh!

Conversation

Uh oh!

Changes in this PR:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!