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

fixed inconsistent output issue #58

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

sumanbyte
Copy link

Description

In addressing a problem, I discovered that:

new BikramSambat()
BikramSambat.fromAD(new Date())
were producing inconsistent results.

The issue arose because, when creating a new instance of BikramSambat, the current date was converted to an ISO string and then sliced to reset the UTC hours to 00:00:00. However, when using the static method directly (BikramSambat.fromAD(new Date())), the UTC handling led to slight discrepancies, causing the date calculation to be offset by one day.

Issue Reference

Inconsistent output #48

Proposed Changes

To ensure consistent results between both approaches, I've applied uniform UTC handling for date calculations, maintaining the same logic for resetting the date to 00:00:00 in UTC.

Screenshots / GIFs / Videos

Checklist

  • Lint rules are passed correctly
  • Code builds correctly
  • Created or updated tests for the new code
  • All existing and new tests passed
  • Pull request title is concise and descriptive.
  • Pull request is linked to the related issue.

Copy link
Member

@ashiishme ashiishme left a comment

Choose a reason for hiding this comment

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

@sumanbyte Thank you so much for taking care of this issue. It's been a long time since I was involved in this project, I totally forgot why we slice the TimeString in the constructor.

I think rather than resetting the TimeString in this PR, I guess it's fine to just remove the slicing from the constructor.

You can do that in this PR and try to run the test and see if all the existing test passes. If yes, you can then add a new test to check if both dates are equal as stated by the original problem.

Let me know what do you think?

@sumanbyte
Copy link
Author

As suggested. i removed the slicing operation from the constructor. instead of using the setUTCHours for consistent output incase of partial days. i changed the getDaysBetweenTwoAdDates function to ignore partial days by using math.floor function instead of math.ceil to correctly count the number of days. i wrote test cases for partial days by checking if it outputs 0 incase of the same current date but in the end date adding 5 minutes to it. Another test case i added was to check if both instance date string and static method date string both matches or not.

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