French Republican Calendar

[ledahulevogyre]

Hi,

I'd like to implement the French Republican calendar system.
The computation seems relatively feasible, but the problem is I don't know where to start. I'm not sure which classes/interfaces I should extend/implement.
ChronoLocalDate, org.threeten.extra.chrono.AbstractDate, LocalDate, ... ?
AbstractChronology, Chronology, ... ?
Can someone spare me the digging analysis ?

Thanks a lot.

[jodastephen]

The best thing is to start by copying one of the existing calendar systems in the ThreeTen-Extra code base. A calendar system needs three classes - the chronology, date and era. JulianChronology might be a reasonable starting point.

[ledahulevogyre]

Thank you Stephen. I have an early draft of the classes in my fork.
https://github.com/ledahulevogyre/threeten-extra/tree/master/src/main/java/org/threeten/extra/chrono
Some tests pass. But I am not sure about :

• Eras. I defined a PRE_REPUBLICAN one with negative numbers, because it's easier. But I think it's strictly wrong. They didn't defined how to represent dates before the day 1. I'm not sure about the meaning of "proleptic" either, in this calendar. Can a chronology leave an era undefined ?
• I haven't tested the years after year 14 either. It's wrong on leap years for centuries etc... but it's not really important for historical dates.
• I considered the sans-culottides as the 13th month. It's not strictly correct. But it's the only way, isn't it ?
• How do I deal with names : for months, for sans-culottides days, for years ? Maybe I should see that later. It's formatting. But should I create a FrenchRevMonth enum for example ?
• How about the classes/variables names that I chose ? Is FrenchRevDate acceptable ?
• I'm stuck with the Ranges now. I don't quite yet understand them I think. Maybe it was a bad idea to extend AbstractDate, because I think there are assumptions about the ChronoField in it (?)

Thank you for any feedback.

[ledahulevogyre]

It seems extending AbstractNileDate classes is more appropriate than AbstractDate, even though France is not exactly near the Nile. Would it be okay?

[jodastephen]

Extending AbstractNileDate does make sense, maybe it could be renamed after your changes are merged.

• we tend to use "BEFORE_XXX" rather than "PRE_XXX" for the era. There is no requirement to support it though - you can just validate to prevent dates before the calendar starts in year 1.

• the 13th month is the best plan for end of year extra days

• names of days/months is a problem that is as yet unsolved in formatting. The design for calendar systems avoids using an enum for months to avoid bloating the API in the JDK.

• Beyond year 14 you probably should pick a calculation method, such as Romme. Just document your choice.

• I'd prefer FrenchRepublic not FrenchRev (don't tend to use abbreviations.

[ledahulevogyre]

I renamed as you suggested.
https://github.com/ledahulevogyre/threeten-extra/blob/master/src/main/java/org/threeten/extra/chrono/FrenchRepublicDate.java
I kept the BEFORE_REPUBLICAN era.
I haven't looked at the comments much.
Almost all the tests pass. I'm not sure about the ones dealing with eras.
Maybe more tests should be added ?

The leap years calculation is every four years starting at 3 (thus years 15, 19, 23, 27…). It's almost what wikipedia calls "Continuous", but without the exceptions for centuries.

PS : I think the "7" in org.threeten.extra.chrono.AbstractDate.getDayOfWeek() [line 176] can be replaced by "lengthOfWeek()".

[ledahulevogyre]

So, what's the next step? Should I create a pull request? Create a new branch on this repo?

Thanks.

[catull]

@ledahulevogyre
Create a PR and one of the maintainers will review it.

One more thing, it is best practice to create a branch first, then make changements in that branch.
Your changes are done in your master branch.

This makes it more complicated for you to track the master branch of the originating repo.

[catull]

@ledahulevogyre

This is a suggestion to "fix up" your workspace, you may challenge it if you want.

• rename your current branch from master to implement-french-republic-calendar

  git branch --move master implement-french-republic-calendar

• add originating upstream repo URL

  git remote add upstream git@github.com:ThreeTen/threeten-extra.git

• create and switch to new branch master

  git checkout -b master

• reset your branch to the upstream master

  git remote update
git reset --hard upstream/master --

• publish your local changes back to origin - your remote repo

  git push origin +master

Now you have two repository in your workspace.
upstream refers to the original repo
origin is the default repo, your own one

Why is this a better situation ?

1. Your master branch can be kept in synch with upstream's master
   You do not modify it, only refresh it when pull requests are merged into upstream/master
2. One feature or bug-fix per branch!
   You could have N different features, or bug-fixes.
3. Refresh local master with git pull upstream master

It is important to keep the branch small and focused, not combine features into 1 branch.
Think of the reviewer, if s/he has to go through dozens of lines to understand the motivation.

The exception is the initial PR, where in this case you provide an implementation for a Chronology + Date + a related test class.

And again, you can always check differences against master.
Always create branches from the master branch, in order to produce pull-able requests.

Good luck.

[catull]

@ledahulevogyre

I have commented your initial commit ledahulevogyre@b87852a
Also commented your second commit ledahulevogyre@5790541

Can't wait to review your pull request, though I am not a maintainer.

[jodastephen]

@ledahulevogyre I'd suggest responding to @catull comments with a new commit, and then follow his excellent instructions for creating a pull request, thanks

[ledahulevogyre]

Thank you both.
Ok, I'm not sure about all the git back magic.
I've just executed the list of commands. But now I think I've messed everything up. My own repo is now the exact same one as the original, and no trace of the implement-french-republic-calendar branch :-(
What have I done wrong ?

[catull]

@ledahulevogyre I will assist you in 3 hours. I don't have access to my workspace at home. I have a check-out of your repo as of yesterday afternoon. You have not lost everything, I can help you.

[catull]

@ledahulevogyre

By the way, this is the status of your tree yesterday afternoon: https://github.com/ledahulevogyre/threeten-extra/tree/5790541d8a507e1d7aaec33bd4825f53ddb795d9

Since all you have now is the master tree, keep it unchanged.

Create a new branch, say implement-french-republic-calendar:
git checkout -b implement-french-republic-calendar

Copy FrenchRepublicChronology.java, FrenchRepublicDate.java and FrenchRepublicEra.java from the tree reference above into src/main/java/org/threeten/extra/chrono/ locally.

Copy TestFrenchRepublicChronology.java into src/test/java/org/threeten/extra/chrono/ locally.

Don't worry about the review findings I filed and you already addressed.
We're going to go through them again, this time properly.

Now that you have the 4 files recovered, add them to a change-set and commit them.
git add src
git commit -m 'Initial check-in, again for the first time'

Push the whole thing up: git push --set-upstream origin implement-french-republic-calendar

Then, create a Pull Request following the guide lines at https://help.github.com/articles/creating-a-pull-request/.

As soon as that is done, I will have a look at the PR.

Good luck.

[ledahulevogyre]

Sorry I had to leave right after my message. Thank you a lot for your support @catull .
I like this solution, because it's much simpler. I understand it. :-)
I even started all over, by cloning the repo again. You can find the branch here : https://github.com/ledahulevogyre/threeten-extra/tree/implement-french-republic-calendar

So, I think it's not ready yet for a PR. I've commented my commit with my concerns : ledahulevogyre/threeten-extra@92c550c

[catull]

This seems to become our modus operandi. I will work on it in .... you guessed right ... about 3 hours from now.

[ledahulevogyre]

Thank you @catull for so much energy into implementing this the most accurately possible. I'm really glad that someone who actually knows something about calendars does this.
The current implementation already surpasses my own professional needs (simply converting historical dates) by far. It means I'll have less time to work on this.

I see you requested more pulls from my own repo. But isn't a branch on this official repo a more appropriate place to continue the development ?

[catull]

There is one pull request open, #3 in your repo.

It is all part of my plan, to assist you with your pull request here.

Right now, you have to bring your branch in shape.
There is plenty more work to do.

E.g. you do not test all range checks yet.
The handling of periods is not implemented.

This repo is very strict with its requirement, you must implement all aspects, otherwise it will not be included.

And that's why we need to carry on in your branch for the moment.
I do not mind.

Sure, you could create a Pull Request, today even.
You are going to receive a lot of findings, which you have to implement anyway.

It is up to you now, either enter a PR or carry on in your repo for a while.
I assist you either way.

[ledahulevogyre]

I have no strong opinion on this either. Of course the master branch should be preserved from developing code. But can't a feature branch on this repo be a better place to centralize development on this calendar? I see this repo already has branches.
Maybe @jodastephen has a personal opinion on what's best (?)

I don't mind approving your PR's when I'm available for that. But it feels strange that I am the one who will "approve" what I don't master.

[jodastephen]

In general, PRs work best when they are almost complete when raised. You can't have shared development on a central branch in the ThreeTen/threeten-extra repo, as you don't have permission.

[catull]

@ledahulevogyre
Stephen (@jodastephen) may correct me on this.

If Stephen is working on a new feature, he is free to create as many branches as he wants and manage them. His own work is also reviewed, I guess.

We outsiders, you and I included, have to clone this repo.
We create our own branch in our repo.
Once that branch is finished, we can create a PR against Stephen's master branch.

He then is still holding the right to reject PRs, with little explanation.

So it is in our best interest to produce a feature-complete PR.
Stephen and other reviewers don't have a lot of time to spend on PRs.

This is the way it works.

[ledahulevogyre]

Ok. @catull I'll give you permission on my repo then. Are you ok with that ? Or do you prefer to go through my approvals ?

[catull]

@ledahulevogyre

I plan to implement all 31 calendars described in the book "Calendrical Calculations" by Reingold and Dershowitz ultimately, in more or less alphabetical order.

Since you have already started to implement the French Republic calendar, this means that you yourself hold a repo; with the power and responsibility of deciding what goes in and what is rejected. So I am a contributor.

Sure, I could completely ignore your repo, do my own thing and take away the joy of contributing code to an exciting project.

I chose collaboration.
That means, you have to excercise some of the responsibility from now on.

I hope this is not putting you off.

[catull]

@ledahulevogyre

Your suggestion is along the lines of what I call collaboration.
I would like to still provide PR requests, since you are the owner of the code, and you have to consciously accept the changes.
Whether I am a repo collaborator or not.
Like I said, I do not mind.

In the end, you swing the bottle of champaign and let the boat sail into the waters of the Atlantic to reach the shore of Stephen's coast, symbolically speaking.

[ledahulevogyre]

I really don't mind if you appropriate the code. I think you have far more expertise, and you've already written more code than me anyway. Because I basically only copy/pasted the Coptic classes and adapted the epoch.
I have no pride to claim. I'm just happy I can compute my dates now.
As for my "responsibility to consciously accept your changes", well I frankly am not familiar with calendar implementations. So I'll accept them gladly, but I'm sorry because I'll certainly fail to accept them consciously.

So working on your repo instead of mine would be fine. And I'll be glad to help if I can.

Thank you again.

[catull]

One day you may discover that this is actually great fun, to implement math properties in a very tangible way.

Okay, it is still abstract because you have to think in weeks, months etc.

I will try to portion the PRs in small bits, digestible.
One feature at a time.

And if that is not your kind of fun, that's okay too.

The French Republican calendar has some very beautiful aspects and patterns.
Like the name of the months in the seasons (-aire vs. -ôse vs. -al vs. -idor).
Or the names of the single days.
I simply like it because the year starts with the automnal equinox, no other calendar I know does that.

[ledahulevogyre]

I share your admiration for the calendar design.