github-profile-summary-cards icon indicating copy to clipboard operation
github-profile-summary-cards copied to clipboard

Fixed decimal UTC time bug

Open SauravDharwadkar opened this issue 2 years ago • 17 comments

fixed #90 floor down value because chart uses hours only

SauravDharwadkar avatar Aug 05 '22 14:08 SauravDharwadkar

Thanks for your work!!

I would like to merge this PR, but maybe we can use Math.round here?

vn7n24fzkq avatar Aug 06 '22 13:08 vn7n24fzkq

Never mind, I am just realize that we use Math.floor can avoid to turn 23.6 to 24, if we use Math.round will need to write some code to deal with some special cases,.

I think we can take this PR.

I will do some more review later, to check there are no other problems.

vn7n24fzkq avatar Aug 06 '22 14:08 vn7n24fzkq

I thought that , utc time 0 , 0.3 , 0.45 in min where 0.6 = 1 https://en.m.wikipedia.org/wiki/List_of_UTC_offsets

So for avoid dealing with 0.5 nunber and 0.3 hour / 30 min i used floor. We can add logic to round robin between hour or ceil if 0.45 . Avoided it becz it'll create un necessary complecation.

SauravDharwadkar avatar Aug 06 '22 14:08 SauravDharwadkar

Or we can simply check value after point , 0 / 3 / 45 and update utc accordingly and use floor for default to avoid incorrect utc format

SauravDharwadkar avatar Aug 06 '22 14:08 SauravDharwadkar

Or we can simply check value after point , 0 / 3 / 45 and update utc accordingly and use floor for default to avoid incorrect utc format

I prefer this way, but throw an exception when the UTC number is invalid. We can check if the value is invalid and throw an exception here to alert the caller the UTC format was incorrect.

And like you said, default to using floor.

vn7n24fzkq avatar Aug 06 '22 14:08 vn7n24fzkq

When i looked chart i realise we need to use number.toFixed(2) // '5.30' this or treat utc as string bcz in chart time should display as e.g. +5.30 or -5.30 But now its just shows 5.3 . Both are same but first glance it look like something is wrong with utc time . Screenshot_20220806-201552_Chrome

SauravDharwadkar avatar Aug 06 '22 14:08 SauravDharwadkar

You are right, would you like to fix it in this PR?

vn7n24fzkq avatar Aug 06 '22 14:08 vn7n24fzkq

Or we can create another issue for this if that need to modify many codes.

vn7n24fzkq avatar Aug 06 '22 14:08 vn7n24fzkq

Seting up utc time on chart as well handling offset simple task , but I'm not sure how will you display warning or handle the wrong utc and dont want to lean whole code. I'll handle chart display and utc offset u do something about warning .

SauravDharwadkar avatar Aug 06 '22 15:08 SauravDharwadkar

Sounds good to me, thanks!

vn7n24fzkq avatar Aug 06 '22 15:08 vn7n24fzkq

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

vn7n24fzkq avatar Aug 06 '22 15:08 vn7n24fzkq

const adjustOffset = function (offset: number, RoundRobin: {offset: number}): number {
    if (offset % 1 == 0) {
        return offset;
        // offset % 1 should be 0.3 or 0.7 but its js and it gives 0.29999 or -0.299999 thats why this frankenstein
    } else if ((offset % 1 > 0.29 && offset % 1 < 0.31) || (offset % 1 < -0.29 && offset % 1 > -0.31)) {
        // toggle up and down between hour
        RoundRobin.offset = (RoundRobin.offset + 1) % 2;
        return RoundRobin.offset === 0 ? Math.floor(offset) : Math.ceil(offset);
    } else if ((offset % 1 > 0.44 && offset % 1 < 0.46) || (offset % 1 < -0.44 && offset % 1 > -0.45)) {
        // distrubute 1 : 3 ratio for 0.45 utc time
        RoundRobin.offset = (RoundRobin.offset + 1) % 4;
        return RoundRobin.offset % 4 == 0 ? Math.floor(offset) : Math.ceil(offset);
    } else {
        // flood down , if utc is given right it will never be executed
        return Math.floor(offset);
    }
};

.
.
 let RoundRobin = {
        offset: 0
    };
.
.
 chartData[24 + adjustOffset(afterOffset, RoundRobin)] += 1;

or use simply this for 0.3 and 0.45

Math.ceil(afterOffset);

SauravDharwadkar avatar Aug 07 '22 06:08 SauravDharwadkar

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

in api IDK how it will display

SauravDharwadkar avatar Aug 07 '22 06:08 SauravDharwadkar

const adjustOffset = function (offset: number, RoundRobin: {offset: number}): number {
    if (offset % 1 == 0) {
        return offset;
        // offset % 1 should be 0.3 or 0.7 but its js and it gives 0.29999 or -0.299999 thats why this frankenstein
    } else if ((offset % 1 > 0.29 && offset % 1 < 0.31) || (offset % 1 < -0.29 && offset % 1 > -0.31)) {
        // toggle up and down between hour
        RoundRobin.offset = (RoundRobin.offset + 1) % 2;
        return RoundRobin.offset === 0 ? Math.floor(offset) : Math.ceil(offset);
    } else if ((offset % 1 > 0.44 && offset % 1 < 0.46) || (offset % 1 < -0.44 && offset % 1 > -0.45)) {
        // distrubute 1 : 3 ratio for 0.45 utc time
        RoundRobin.offset = (RoundRobin.offset + 1) % 4;
        return RoundRobin.offset % 4 == 0 ? Math.floor(offset) : Math.ceil(offset);
    } else {
        // flood down , if utc is given right it will never be executed
        return Math.floor(offset);
    }
};

.
.
 let RoundRobin = {
        offset: 0
    };
.
.
 chartData[24 + adjustOffset(afterOffset, RoundRobin)] += 1;

I think we can take this code, and after that, we don't need to throw exceptions, tho.

vn7n24fzkq avatar Aug 07 '22 08:08 vn7n24fzkq

By the way, about the warning, I will just throw an exception like this.

if(!IsValidUTCTime(utcTime){
    throw Exception ("The UTC format is invalid, it should be ......")
}

in api IDK how it will display

We don't care about the error display here, just throw an exception.

And Math.ceil(afterOffset); is accepted too, if we use this way, I will take the UTC validation part.

vn7n24fzkq avatar Aug 07 '22 08:08 vn7n24fzkq

@SauravDharwadkar Thanks for your hard work!

Are you still working on this? Please let me know if there is any problem.

vn7n24fzkq avatar Aug 10 '22 03:08 vn7n24fzkq

was busy . i didn't test negative value bcz process.arg not work with negative value

SauravDharwadkar avatar Aug 12 '22 11:08 SauravDharwadkar

Hi, @SauravDharwadkar. Are you still working on this? Since this PR is almost done, if you don't mind I can take over the rest part and keep your commit in the commit history.

vn7n24fzkq avatar Nov 10 '22 03:11 vn7n24fzkq

You can take over , sorry for late reply busy last couple of months

SauravDharwadkar avatar Dec 16 '22 04:12 SauravDharwadkar

Solved in #123

vn7n24fzkq avatar Jan 07 '23 10:01 vn7n24fzkq