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

Implement the Wondrous/Badi calendar #652

Closed
glittle opened this issue Feb 19, 2017 · 28 comments
Closed

Implement the Wondrous/Badi calendar #652

glittle opened this issue Feb 19, 2017 · 28 comments
Milestone

Comments

@glittle
Copy link
Collaborator

glittle commented Feb 19, 2017

How can I help to implement the Badi (Wondrous) calendar? It is the official calendar of the Baha'i Faith, and has fairly simple rules.

I've implemented the calendar in .NET and Javascript in a number of systems already, and would like to see it in NodaTime as well.

@glittle glittle changed the title Implement the Badi calendar Implement the Wondrous/Badi calendar Feb 19, 2017
@jskeet
Copy link
Member

jskeet commented Feb 20, 2017

I'm quite happy for us to do this, but I'd rather not do it before 2.0. It would probably be reasonable to start a Badi branch though.

Implementing a calendar basically consists of implementing a YearMonthDayCalculator - have a look at the abstract methods in that for examples. If you're able to either create a pull request or provide appropriate information so I can implement it myself, that would be great - although in the latter case I probably won't get round to it until the second half of 2017.

@glittle
Copy link
Collaborator Author

glittle commented Feb 20, 2017

Thanks, Jon! I should be able to do a pull request to get it started. I probably won't start for a couple of weeks yet, as I've got other projects to tend to first.

Is the main branch here (https://github.com/nodatime/nodatime) destined to be version 2? Or should I look somewhere else? (If you could "start the branch" and point me there, that might save us both some time!)

@jskeet
Copy link
Member

jskeet commented Feb 20, 2017

Yes, that's the branch that will be 2.0. I've just created a new "badi" branch - if you create pull requests targeting that, that would be ideal.

@glittle
Copy link
Collaborator Author

glittle commented Feb 22, 2017

I just tried to push to the "badi" branch and got this: "remote: Permission to nodatime/nodatime.git denied to glittle". I'm a novice with git... is there something I need to do instead, or do you need to give me access to that branch?

I'm wondering if I should have forked your "badi" branch and done my work there. Is that right?

@jskeet
Copy link
Member

jskeet commented Feb 22, 2017 via email

@glittle
Copy link
Collaborator Author

glittle commented Mar 6, 2017

Unfortunately, I've run into what might be a show-stopping roadblock.

The underlying YearMonthDay struct does not support month numbers beyond 16. The Wondrous calendar requires 19 months (+ another for Ayyam-i-Ha).

I've committed working code into the badi branch in my fork (https://github.com/glittle/nodatime).

Until a workaround or another approach is identified, I cannot do any more development or testing. There are a number of tests that I want to add, but can't until the YearMonthDay struct works.

@glittle
Copy link
Collaborator Author

glittle commented Mar 6, 2017

Unfortunately, this looks like a case of optimization that later proves to be 'premature'!

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

Well, it wasn't premature in that it definitely improved performance - it just relied on an assumption that would apparently be invalid under the Wondrous calendar. I'll have a look and see whether it's feasible to give the month number another bit...

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

Hmm. It looks like we could reduce the number of "day bits" to 5, allowing up to 32 days per month, and up to 32 months per year. I don't think we can reduce the year range.

I'm somewhat nervous about a maximum of 32 days per month though, given that there are up to 31 in the Gregorian calendar. Will have to think about this a bit more and do some more research. It would be annoying to change this for the Wondrous calendar in a way that then made other calendars impossible to implement...

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

https://en.wikipedia.org/wiki/List_of_calendars has a list of calendars we'd want to consider.
There's a lot to look through, but will do what I can...

@glittle
Copy link
Collaborator Author

glittle commented Mar 6, 2017

From the comment in the code, it looks like the year could be adjusted...

private const int YearBits = 15; // 32K range; only need -10K to +10K.

If that were reduced to 14, you'd have a 16K range, still bigger than what is needed.

Would changing the bits affect current implementations? If someone serializes a date, is it stored in the internal bit pattern, or otherwise?

My first thought was to overload the YearMonthDate class and adjust the internal representation when used for this calendar, but it is a struct. I was also not sure of the implications of doing so!

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

No, we need -9998 to 9999, which is nearly 20K of range.

Changing the bit pattern now would be just about okay - doing it after 2.0 has been released wouldn't be. So we definitely need to get it right now :)

Adjusting the internal representation on a per calendar basis would inflict quite a nasty performance penalty, unfortunately. The reason for using this representation is to make it really really quick to get at each component. Adding any branching would slow things down quite a bit :(

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

Ah, I'm silly - we can just add another bit for months... we've got lots of space, as it were, in YearMonthDay. YearMonthDayCalendar is where it gets tighter, and currently we have 7 bits for the calendar system ID... I think it's absolutely fine to reduce that to 6. I doubt that we'll need more than 64 calendar systems, ever!

Will do this in the master branch, and then would could rebase your branch - are you sufficiently comfortable with git to do that part yourself? (I'll cc you on the pull request, so you'll know when it's done.)

@glittle
Copy link
Collaborator Author

glittle commented Mar 6, 2017

Great news! I'll see what I can do for the rebase...

@jskeet
Copy link
Member

jskeet commented Mar 6, 2017

It's not in yet :) But when it is in, I'd expect you to be able to do:

git checkout master
git pull upstream master
git checkout badi
git rebase master
git push -f origin badi

That's assuming you have remotes setup as upstream=nodatime, origin=your fork.

The rebase may have merge conflicts, of course, at which point things get slightly trickier.

@jskeet
Copy link
Member

jskeet commented Mar 7, 2017

Right, that PR is now merged. Good luck :)

@glittle
Copy link
Collaborator Author

glittle commented Mar 7, 2017

I had merge conflicts. I also had a number of commits that I'd rather squash. To keep it simple, I've "manually stashed" the changes and files that I need to touch then deleted my fork, and reforked nodatime. However, I think your "badi" branch is no longer up-to-date. Could you update it, or should I not use that branch?

@jskeet
Copy link
Member

jskeet commented Mar 7, 2017

Have rebased the badi branch.

@glittle
Copy link
Collaborator Author

glittle commented Mar 7, 2017

Thanks. I think I'm on track now... The only real setback is that neglected to "stash" my unit tests file. It will take a while to rebuild all the tests. :(

@glittle
Copy link
Collaborator Author

glittle commented Mar 7, 2017

How do I define DateTimeFormat.MonthNames for a new calendar? I haven't found it for any of the other calendars...

@jskeet
Copy link
Member

jskeet commented Mar 7, 2017

Unfortunately, the .NET support for calendar localization is pretty poor. We effectively don't support localization on a per-calendar basis, because .NET just isn't geared up for it. At some point we'd like to support names via CLDR, but it's not on the immediate roadmap. For now, I'm afraid that will leave the Wondrous calendar without text support - just like there's no reliable text handling for the Hebrew calendar :(

@malcolmr malcolmr added this to the 2.1-consider milestone Mar 15, 2017
@jskeet
Copy link
Member

jskeet commented Oct 16, 2017

@glittle: Can I check whether this is still of interest? I'm doing a bit of housekeeping, and if you've now decided not go ahead with this, I'll close this issue and related ones.

@glittle
Copy link
Collaborator Author

glittle commented Oct 16, 2017

Hi @jskeet. I was actually thinking of Noda Time recently and was planning to try again and see whether I could get it finished. So I'd like to keep it open for a bit longer. I expect to work on it in November.

Since version 2.2 is now current, should I start over with a current codebase? Can you bring the 'badi' branch up-to-date for me?

Thanks!

@jskeet
Copy link
Member

jskeet commented Oct 16, 2017

I've brought the badi branch up to date with 2.2.x. Is that what you're after, or would you like me to rebase it to master? (2.2.x will give you some more stability while developing; master would be more up-to-date.)

Good to hear you're still considering it :)

@glittle
Copy link
Collaborator Author

glittle commented Oct 16, 2017

As per my previous comments in this thread, I'm a novice with the nuances of git! What would you recommend for this?

@jskeet
Copy link
Member

jskeet commented Oct 17, 2017

Probably simplest to work on a 2.2 base at the moment then. I can handle the rebasing later to bring it up to master when you're ready.

@glittle
Copy link
Collaborator Author

glittle commented Oct 18, 2017

Took me a while to figure out why the instructions here (http://nodatime.org/developer/building) were not working!

Turns out that this file (https://github.com/nodatime/nodatime/blob/badi/global.json) was not updated, so 'dotnet' was reverting to version 1 whenever I was in the solution's folder! Back on track again now...

@glittle
Copy link
Collaborator Author

glittle commented Nov 15, 2017

This thread can likely be closed now as #689 is also about the Wondrous calendar.

@glittle glittle closed this as completed Nov 15, 2017
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

No branches or pull requests

3 participants