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: update months total days data #80

Merged
merged 1 commit into from
Jan 26, 2025

Conversation

aj3sh
Copy link
Member

@aj3sh aj3sh commented Jan 13, 2025

Description

Related Issue

N/A

Type of Change

Please mark the appropriate option below to describe the type of change your pull request introduces:

  • Bug fix
  • New feature
  • Enhancement
  • Documentation update
  • Refactor
  • Other (please specify)

Checklist

  • I have used pre-commit hooks.
  • I have added/updated the necessary documentation on README.md.
  • I have updated CHANGELOG.md for the significant changes.
  • I have added appropriate test cases (if applicable) to ensure the changes are functioning correctly.
  • My pull request has a clear title and description.

Additional Notes

[Add any additional notes or context that you think the reviewers should know about.]

By submitting this pull request, I confirm that I have read and complied with the contribution guidelines of this project.

@aj3sh aj3sh requested a review from sugat009 January 13, 2025 09:23
Copy link
Collaborator

@sumanchapai sumanchapai left a comment

Choose a reason for hiding this comment

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

Looks good. I checked 2082 data with NepaliPatro.

A question about whether our versioning is semantic. Looks like we're marking this as a patch release. I don't know. Is this a patch, or is this a breaking change or a backwards incompatible change?

@aj3sh
Copy link
Member Author

aj3sh commented Jan 14, 2025

Looks good. I checked 2082 data with NepaliPatro.

A question about whether our versioning is semantic. Looks like we're marking this as a patch release. I don't know. Is this a patch, or is this a breaking change or a backwards incompatible change?

@sumanchapai, I consider this as a bug, thus making it as a patch release. I dont think the user using this package will have a breaking change.

If you have any suggestion, let us know. :-)

@sumanchapai
Copy link
Collaborator

@sumanchapai, I consider this as a bug, thus making it as a patch release. I dont think the user using this package will have a breaking change.

If you have any suggestion, let us know. :-)

I see. I think a patch release is good. But it seems to me like a case can be made that it's a breaking change as well. If someone using this package as a library in their project was relying on a particular month that got updated having certain number of days, this release could break their code.

Imagine a program (slightly impractical) that asks the user to guess the number of days in Baisakh 2082, if they guessed incorrectly it runs rm -rf on their computer. Before the update, 30 was the correct number to guess. They make a seemingly harmless update (since it's a patch, not a breaking change). Now they guess 30 again and they're dead. Lol.

@aj3sh
Copy link
Member Author

aj3sh commented Jan 14, 2025

@sumanchapai Nice one. But is it okay to bump major version on every date correction. I'm still not sure that the future data is correct. Might have to bump major versions in those cases.

@sumanchapai
Copy link
Collaborator

I agree. Patch release is good. Major version release in every correction seems impractical.

@sugat009
Copy link
Contributor

Since this package's main feature is dates, changes such as date fixes should be minor releases at least, and adding new dates should be major releases.

@aj3sh
Copy link
Member Author

aj3sh commented Jan 22, 2025

Since this package's main feature is dates, changes such as date fixes should be minor releases at least, and adding new dates should be major releases.

Should we release minor version without a feature? Following the semantic versioning, fixes should be in patch version.

@aj3sh
Copy link
Member Author

aj3sh commented Jan 26, 2025

This PR is taking too long. Merging this as it has the critical bugfix.

@aj3sh aj3sh merged commit eaa36ef into opensource-nepal:main Jan 26, 2025
5 checks passed
@aj3sh aj3sh deleted the fix-month-days branch January 26, 2025 06:40
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.

3 participants