Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add missing page query parameter to getReviewComments #12

Merged
merged 1 commit into from
May 19, 2022
Merged

Add missing page query parameter to getReviewComments #12

merged 1 commit into from
May 19, 2022

Conversation

Fs00
Copy link

@Fs00 Fs00 commented Mar 26, 2022

The page parameter was missing, causing the inability to fetch all comments for reviews with more than 30 comments (in OctoDroid this was causing an infinite loop of requests to the first page).

A sidenote: have you ever considered putting this SDK as a Gradle subproject in OctoDroid repo? It would easier to contribute to it (no double PRs needed) and it would spare you the need to release a new version of the library every time :)

@Fs00
Copy link
Author

Fs00 commented May 19, 2022

@maniac103 Can we have this fix included before the next release?
(btw, the infinite loop of requests caused by this issue can be seen in TeamNewPipe/NewPipeExtractor#810 (review))

@maniac103 maniac103 merged commit 7771942 into maniac103:gh4a-rebase May 19, 2022
@maniac103
Copy link
Owner

I just created a 0.7.0.9 tag.

@maniac103
Copy link
Owner

A sidenote: have you ever considered putting this SDK as a Gradle subproject in OctoDroid repo? It would easier to contribute to it (no double PRs needed) and it would spare you the need to release a new version of the library every time :)

I wanted to keep the possibility to do upstream PRs, which isn't possible when importing the project into OctoDroid.

@Fs00
Copy link
Author

Fs00 commented May 19, 2022

I just created a 0.7.0.9 tag.

Thanks. Would you mind updating OctoDroid to use the new version of the bindings? I'm not at home these days so I'm limited in what I can do.
If you have some time, I think that before releasing a new version of the app it might be worth looking into slapperwan/gh4a#1179, which is quite a severe issue that requires changes to the bindings.

I wanted to keep the possibility to do upstream PRs, which isn't possible when importing the project into OctoDroid.

Makes sense, but I'm wondering if this possibility is still relevant given that the upstream project seems to be dead...

@maniac103
Copy link
Owner

Would you mind updating OctoDroid to use the new version of the bindings? I'm not at home these days so I'm limited in what I can do.

I just tried, but it looks like #13 was incomplete because the returned binary is interpreted as JSON now, which fails. I guess we'll need a Converter.Factory for byte[] data, similar to StringResponseConverterFactory.

@Fs00
Copy link
Author

Fs00 commented May 19, 2022

I've seen that you added the missing converter factory, thanks! I wasn't aware of that requirement.

@maniac103
Copy link
Owner

I'm not yet sure it'll work though, needs more investigation.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants