-
-
Notifications
You must be signed in to change notification settings - Fork 23.4k
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
base: master
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this 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.
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 |
Co-authored-by: Rick Staa <rick.staa@outlook.com>
@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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
tests/fetchStats.test.js
Outdated
@@ -253,4 +270,29 @@ describe("Test fetchStats", () => { | |||
rank, | |||
}); | |||
}); | |||
|
|||
it("should get commits of provided year", async () => { |
There was a problem hiding this comment.
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:
github-readme-stats/tests/fetchStats.test.js
Line 127 in af6553e
it("should fetch correct stats", async () => { |
github-readme-stats/tests/fetchStats.test.js
Line 216 in af6553e
it("should fetch total commits", async () => { |
and
github-readme-stats/tests/fetchStats.test.js
Line 191 in af6553e
it("should fetch and add private contributions", async () => { |
There was a problem hiding this comment.
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
github-readme-stats/tests/fetchStats.test.js
Line 193 in 4b15721
it("should get present year commits when provide no year", async () => { |
There was a problem hiding this comment.
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.
Maybe |
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 👍🏻. |
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 🤔. |
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 |
There was a problem hiding this 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.
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Deployment failed with the following error:
|
There was a problem hiding this 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.
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Co-authored-by: Alexandr Garbuzov <qwerty541zxc@gmail.com>
Fixed as requested, @qwerty541. Thanks for spotting and reporting my mistakes! |
There was a problem hiding this 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.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Add custom year feature as in issue #764
now requesting
/api?username=vzsky&year=2021
should work!