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

Refactored struct #19

Closed
wants to merge 4 commits into from
Closed

Refactored struct #19

wants to merge 4 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 31, 2024

  • Refactored dateConverter with new Date struct.
  • Added a Calendar flag in Date struct.
  • Added Weekday Method to get week.
  • Adding tests of the method.

@ghost ghost marked this pull request as ready for review March 31, 2024 13:15
@aj3sh aj3sh requested review from aj3sh and sugat009 and removed request for aj3sh April 1, 2024 07:34
@ghost ghost marked this pull request as draft April 1, 2024 08:10
@sugat009
Copy link
Contributor

sugat009 commented Apr 1, 2024

@thearjunneupane Can you please squash your commits so that this PR only has 1 commit?

@aj3sh
Copy link
Member

aj3sh commented Apr 1, 2024

Hi @thearjunneupane, I appreciate your contribution. From my perspective, I don't see a need for this refactoring.

The dateConverter module is very lightweight, and I prefer to keep it that way. All the detailed features are available in the nepalitime module, including the weekday. Here are some examples of date conversion using the nepalitime module.

// english to nepali
enTime1 := time.Date(2023, 1, 29, 0, 0, 0, 0, nepalitime.GetNepaliLocation())
npTime1, _ := nepalitime.FromEnglishTime(enTime1)
fmt.Println(npTime1.Year(), npTime1.Month(), npTime1.Day(), npTime1.Weekday())

// nepali to english
npTime2, _ := nepalitime.Date(2079, 10, 15, 14, 29, 6, 7)
enTime2 := npTime2.GetEnglishTime()
fmt.Println(enTime2.Year(), enTime2.Month(), enTime2.Day(), enTime2.Weekday())

cc: @sugat009

@ghost
Copy link
Author

ghost commented Apr 1, 2024

Hi @aj3sh
Thank you for your feedback. I understand your perspective on keeping the dateConverter module lightweight, and I appreciate your preference for simplicity.
After reviewing your examples with the nepalitime module, I can see that it provides comprehensive date conversion features, including the weekday, which meets our requirements effectively. Therefore, I agree that there might not be a necessity for the additional refactoring.

@ghost ghost closed this Apr 1, 2024
@aj3sh
Copy link
Member

aj3sh commented Apr 1, 2024

@thearjunneupane, go-nepali is still in beta and requires further enhancements and improvements. For instance, we can simplify the initialization process of nepalitime as mentioned in my example. Additionally, not all features are covered in the documentation.

Feel free to contribute and create an issue if you believe any enhancements are necessary. We are open to discussion.

This pull request was closed.
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