Skip to content
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

feat: DRY todo done, add .gitignore(python) for this repo #101

Merged
merged 1 commit into from
Apr 9, 2021
Merged

feat: DRY todo done, add .gitignore(python) for this repo #101

merged 1 commit into from
Apr 9, 2021

Conversation

yihong0618
Copy link
Contributor

@yihong0618 yihong0618 commented Apr 9, 2021

This PR completes the TODO

api = url + "?page=0&per_page=" + str(per_page)
def _get_all_repo_names(self, url, page=1):
per_page = 60
api = url + f"?page={page}&per_page=" + str(per_page)
Copy link
Owner

@Yikun Yikun Apr 9, 2021

Choose a reason for hiding this comment

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

A quick question: the page start is changed from 0 to 1, start 0 and start1, have same result(include github and gitee), right?

Copy link
Contributor Author

@yihong0618 yihong0618 Apr 9, 2021

Choose a reason for hiding this comment

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

I think its start 1, start 0 is kind of wrong, because I also found this problem in my other repo

I think GitHub apis may have the same logic.
image

page += 1
items = response.json()

return names + self._get_all_repo_names(url, page=page+1)
Copy link
Owner

Choose a reason for hiding this comment

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

nice recursion

@Yikun
Copy link
Owner

Yikun commented Apr 9, 2021

Thanks for your contribution, I do some basic test on #102 . It works.

The code is LGTM, I think it's ok to merge after the question anwsered.

@yihong0618
Copy link
Contributor Author

yihong0618 commented Apr 9, 2021

Thanks for your contribution, I do some basic test on #102 . It works.

The code is LGTM, I think it's ok to merge after the question anwsered.

But I am not 100% sure, you can test by change the per page, starts from 1 seems to make more sense.

@yihong0618
Copy link
Contributor Author

yihong0618 commented Apr 9, 2021

@Yikun

I search the GitHub API Doc, found it is from 1.

see: https://docs.github.com/cn/rest/guides/traversing-with-pagination

But not sure for gitee

@Yikun
Copy link
Owner

Yikun commented Apr 9, 2021

ok,both gitee and github note that the start page is 1
github: This makes sense, since by default, all paginated queries start at page 1.
gitee: https://gitee.com/api/v5/swagger#/getV5UsersUsernameRepos

@yihong0618
Copy link
Contributor Author

恩,gitee和github都没问题:
github: This makes sense, since by default, all paginated queries start at page 1.
gitee: https://gitee.com/api/v5/swagger#/getV5UsersUsernameRepos

哈哈,算是修了一个潜在的 bug~

@Yikun Yikun merged commit 869c368 into Yikun:master Apr 9, 2021
@Yikun
Copy link
Owner

Yikun commented Apr 9, 2021

Merged, : )

@yihong0618
Copy link
Contributor Author

Merged, : )

Thanks.

@Yikun
Copy link
Owner

Yikun commented Apr 9, 2021

目前贡献者的CI仅跑了flake8,贡献体验不太好,在#103 跟踪一下,用例集成

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