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

Issue #764: Add custom year #2107

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Issue #764: Add custom year #2107

wants to merge 28 commits into from

Conversation

vzsky
Copy link

@vzsky vzsky commented Oct 4, 2022

Add custom year feature as in issue #764

now requesting /api?username=vzsky&year=2021 should work!

@vercel
Copy link

vercel bot commented Oct 4, 2022

Someone is attempting to deploy a commit to the github readme stats Team on Vercel.

A member of the Team first needs to authorize it.

@rickstaa rickstaa linked an issue Oct 8, 2022 that may be closed by this pull request
Copy link
Collaborator

@rickstaa rickstaa left a comment

Choose a reason for hiding this comment

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

Overall the idea is okay. There, however, is one bug, and the documentation and tests should be updated.

src/fetchers/stats-fetcher.js Outdated Show resolved Hide resolved
src/fetchers/stats-fetcher.js Outdated Show resolved Hide resolved
@vzsky
Copy link
Author

vzsky commented Oct 10, 2022

Edit as requested. I'm not sure if there's a better way to write the test. Please let me know if you have some advices

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
vzsky and others added 3 commits October 13, 2022 21:45
Co-authored-by: Rick Staa <rick.staa@outlook.com>
@rickstaa
Copy link
Collaborator

rickstaa commented Oct 15, 2022

@vzsky I checked your PR, and there were still some bugs. I fixed these bugs in eb445fa and af6553e. I made some comments for you to understand why I made these changes. Please review these commits 👍🏻.

One tip: You can run the project locally using the vercel dev command (see https://github.com/anuraghazra/github-readme-stats/blob/master/CONTRIBUTING.md#local-development). Doing this will allow you to debug your code locally using vscode (see vercel/vercel#2864 (comment)).

@codecov
Copy link

codecov bot commented Oct 15, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9406466) 98.02% compared to head (de4cefa) 96.66%.
Report is 18 commits behind head on master.

❗ Current head de4cefa differs from pull request most recent head b60aa8f. Consider uploading reports for the commit b60aa8f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2107      +/-   ##
==========================================
- Coverage   98.02%   96.66%   -1.36%     
==========================================
  Files          27       22       -5     
  Lines        6275     3811    -2464     
  Branches      549      328     -221     
==========================================
- Hits         6151     3684    -2467     
- Misses        121      125       +4     
+ Partials        3        2       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tests/fetchStats.test.js Outdated Show resolved Hide resolved
@@ -253,4 +270,29 @@ describe("Test fetchStats", () => {
rank,
});
});

it("should get commits of provided year", async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I removed the other test, which tested whether the current year is returned when no year is supplied. I did this because this is already tested in:

it("should fetch correct stats", async () => {

it("should fetch total commits", async () => {

and

it("should fetch and add private contributions", async () => {

Copy link
Collaborator

@rickstaa rickstaa Oct 15, 2022

Choose a reason for hiding this comment

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

@anuraghazra, are you okay with this implicit testing, or do you want to have an explicit test like @vzsky did in

it("should get present year commits when provide no year", async () => {

Copy link
Author

Choose a reason for hiding this comment

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

I did that just to be sure that the mocked function works fine. I think deleting that is probably okay since the mocked function was tested and is not likely to get changed.

tests/fetchStats.test.js Outdated Show resolved Hide resolved
tests/fetchStats.test.js Outdated Show resolved Hide resolved
@rickstaa rickstaa self-requested a review October 15, 2022 10:05
@lionkor
Copy link

lionkor commented Jan 15, 2024

Maybe last year still implies "last, but not this, year". How about 1 year?

@francois-rozet
Copy link
Collaborator

(one year) or (1 year) is fine, but would need to make it uniform.

Anyway, I find myself unable to check whether the feature works or not. I am not a JS dev, and don't get how to render a "dummy" card locally 😅

@rickstaa
Copy link
Collaborator

(one year) or (1 year) is fine, but would need to make it uniform.

Anyway, I find myself unable to check whether the feature works or not. I am not a JS dev, and don't get how to render a "dummy" card locally 😅

@francois-rozet thanks for reviewing this PR ❤️‍🔥🚀. I will do the final review, including testing if it works, but your approval will give me the confidence to merge it 👍🏻.

@rickstaa
Copy link
Collaborator

(one year) or (1 year) is fine, but would need to make it uniform.

Anyway, I find myself unable to check whether the feature works or not. I am not a JS dev, and don't get how to render a "dummy" card locally 😅

I agree that it should be uniform. I used last year for the contributors count since it was clear that it meant 365 days in the past, especially since the year option will show (2024) etc. Then again, I'm not a native English speaker, so it might be better to change the text 🤔.

@vzsky
Copy link
Author

vzsky commented Jan 15, 2024

Thanks to @francois-rozet for the comments and @rickstaa for offering to do the final review.

I have edited according to the comments from @francois-rozet.

About the display, I think both (last year) and (one year) are fine, but I prefer (one year) a little more. Also, I am also not a native. If there is a consensus then it should be easy to fix the code (and the tests?).

edit: implementation went with last year

Copy link
Collaborator

@qwerty541 qwerty541 left a comment

Choose a reason for hiding this comment

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

Hey, @vzsky! Thanks for your pull request! I left some comments, please check it, after it will be resolved i'm okay to give my approval.

src/fetchers/stats-fetcher.js Outdated Show resolved Hide resolved
src/cards/types.d.ts Outdated Show resolved Hide resolved
src/fetchers/stats-fetcher.js Outdated Show resolved Hide resolved
api/index.js Outdated Show resolved Hide resolved
src/fetchers/stats-fetcher.js Outdated Show resolved Hide resolved
tests/fetchStats.test.js Outdated Show resolved Hide resolved
src/cards/stats-card.js Outdated Show resolved Hide resolved
tests/fetchStats.test.js Outdated Show resolved Hide resolved
vzsky and others added 4 commits January 23, 2024 19:58
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Copy link

vercel bot commented Jan 23, 2024

Deployment failed with the following error:

The provided GitHub repository does not contain the requested branch or commit reference. Please ensure the repository is not empty.

Copy link
Collaborator

@qwerty541 qwerty541 left a comment

Choose a reason for hiding this comment

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

@vzsky Thanks for your efforts! The code looks much better now, but still needs a little polish. Please check my comments. I think that will be last changes request.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
src/cards/stats-card.js Outdated Show resolved Hide resolved
vzsky and others added 4 commits January 25, 2024 23:11
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
@vzsky
Copy link
Author

vzsky commented Jan 25, 2024

Fixed as requested, @qwerty541.

Thanks for spotting and reporting my mistakes!

Copy link
Collaborator

@qwerty541 qwerty541 left a comment

Choose a reason for hiding this comment

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

@vzsky No problem, thats my responsibility as collaborator to keep repository clean, thanks for your awesome contribution! I approve this pull request in it's current state. I see that @rickstaa already approved this pull request, but his review is a bit outdated. I will request a new one, but we have to wait a little since at the moment he is busy with master thesis defense, i'm counting on your patience for a little more.

@1chooo
Copy link

1chooo commented Jun 27, 2024

Hi @qwerty541,

I would like to ask if there are any ways to speed up the review of these PRs. This issue has been ongoing for a long time, and I hope it can be resolved because the number of commits doesn't match up. There are even cases where the total number of commits is less than the commits in the current year, which is quite odd!

If there's anything I can do to help, please let me know! Thank you to the development team for your hard work!

@lionkor
Copy link

lionkor commented Jun 28, 2024

image

2 years in review <3 truly a pinnacle of engineering

just merge it if it works, or find other maintainers.

@1chooo
Copy link

1chooo commented Jul 6, 2024

Hi @qwerty541,

I would like to ask if there are any ways to speed up the review of these PRs. This issue has been ongoing for a long time, and I hope it can be resolved because the number of commits doesn't match up. There are even cases where the total number of commits is less than the commits in the current year, which is quite odd!

If there's anything I can do to help, please let me know! Thank you to the development team for your hard work!

cc @rickstaa

Copy link

@1chooo 1chooo left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation. hacktoberfest hacktoberfest-accepted ⭐ top pull request Top pull request. stats-card Feature, Enhancement, Fixes related to stats the stats card.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Showing number of commits from last 1 year instead of the current year Custom Year?
7 participants