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

fix(core, achievements): Account age due to leap years #501

Merged
merged 5 commits into from
Aug 26, 2021
Merged

fix(core, achievements): Account age due to leap years #501

merged 5 commits into from
Aug 26, 2021

Conversation

spenserblack
Copy link
Contributor

Sorry to spam PRs (#499). Looks like I can't re-open a PR after a force-push.

This creates a new date created from the diff. The UTC year of that new date is used. Because it's created from a diff, that date is equal to the diff + 1970, so 1970 is subtracted from the year to get the year diff.

I wasn't able to write any automated tests (looks like a lot of refactoring would have to happen for the plugins if we're going to test them), but I've informally tested over leap years, and generated a metrics .svg.

As a simple check, you can do this

#!/usr/bin/env node
const now = new Date('2021-08-24T00:00:00Z');
const created = new Date('2017-08-25T00:00:00Z');

console.log('now:', now);
console.log('created:', created);

console.log('years (old way):', (now - created) / (365 * 24 * 60 * 60 * 1000));
console.log('years (new way):', new Date(now - created).getUTCFullYear() - 1970);

By converting the diff to a date, the date since 1970 is calculated. The
year of that date minus 1970 would then equal the number of years that
have passed, taking leap years into account.

The "Member" achievement (number of years) has been adjusted to use the
years calculated from the core.
@spenserblack
Copy link
Contributor Author

Note

Following discussion in #499, it would probably be preferable if the "Member" achievement could increment daily, which this breaks.

@@ -143,7 +143,7 @@ export default async function({list, login, data, computed, imports, graphql, qu

//Member
{
const value = computed.registered.diff
const { years: value } = computed.registered
Copy link
Owner

Choose a reason for hiding this comment

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

const { years, months } = computed.registered
const value = years + months/12

May be good enough to mitigate huge progress steps.
If I'm not too bad at maths it would result in 8/4/4/2/2% steps, so it's kind of acceptable if daily increase are tedious to achieve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just adding days / 365.25. It's not a perfectly precise way of calculating the incremental change daily, but I think the change is so small that it's not necessary to add/remove a day if it's a leap year.

source/plugins/core/index.mjs Outdated Show resolved Hide resolved
@lowlighter
Copy link
Owner

Happy "GitHub day" (without leap years) by the way 🥳

Dividing days by `365.25` is a fairly rough way to do things, but the
percentage difference should be imperceptible enough this it should be
visually adequate.
In case the beginning of time according to JavaScript ever changes, this will keep the years calculation consistent.

Co-authored-by: Simon Lecoq <22963968+lowlighter@users.noreply.github.com>
@spenserblack
Copy link
Contributor Author

Happy "GitHub day" (without leap years) by the way

Thank you!
Just made some changes. With these changes to the calculations, hopefully there's a good balance of accuracy and visual quality. Although I'm feeling kind of braindead today TBH, so I may have to review my own code later 😆

@spenserblack
Copy link
Contributor Author

I tried to keep the git history clean (hence the force-pushes in #499), but the git history has gotten a bit messy again. Please let me know if you want me to rebase at all.

@Nixinova
Copy link
Contributor

I tried to keep the git history clean (hence the force-pushes in #499), but the git history has gotten a bit messy again. Please let me know if you want me to rebase at all.

PRs are squash merged so it should be fine either way

@lowlighter lowlighter merged commit e3cd805 into lowlighter:master Aug 26, 2021
@lowlighter
Copy link
Owner

Thanks for your contribution 👍 !

JayantGoel001 referenced this pull request in JayantGoel001/metrics Aug 26, 2021
fix(core, achievements): Account age due to leap years (#501)
@spenserblack spenserblack deleted the bugfix/date-diff branch August 26, 2021 12:15
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants