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

Use holiday names from code du travail #6

Merged
merged 7 commits into from
May 15, 2020
Merged

Conversation

AntoineAugusti
Copy link
Member

@AntoineAugusti AntoineAugusti commented May 13, 2020

Dear reviewers,

We've got an important matter to discuss. France is watching us.

See discussion on data.gouv.fr. The question was: should we name bank holidays according to the code du travail or should we stick to names used by people?

Switching to names from code du travail

Pros:

  • we don't have to pick names
  • people have trouble to associate dates and names (When is Victoire des alliés again?)

Cons:

  • it's a breaking change
  • it's farther away from how people speak

To discuss:

  • Rename methods?
  • Document name <-> method mapping?

@AntoineAugusti AntoineAugusti requested a review from a team May 13, 2020 22:31
@AntoineAugusti AntoineAugusti marked this pull request as ready for review May 13, 2020 22:32
@abulte abulte requested a review from geoffreyaldebert May 14, 2020 07:05
@abulte
Copy link

abulte commented May 14, 2020

Like I said, I kinda like the "code du travail" names and it's an obvious way to justify our choices.

@geoffreyaldebert
Copy link

Very tough decision indeed...

I agree with choosing "code du travail" names.
If we want to be super precise, we could use two fields, one "nom_jour_ferie" related to "code du travail" and another one like "nom_usage_jour_ferie" related to "service-public" denomination.
But @AntoineAugusti I'm afraid to ask you that. I could take this on me after this is merge if you want.

Copy link

@geoffreyaldebert geoffreyaldebert left a comment

Choose a reason for hiding this comment

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

Approved with remark of @abulte taking into account (changing function names)

@AntoineAugusti
Copy link
Member Author

I updated all the method names, documented the mapping, bumped the version and added a CHANGELOG 😄

README.md Outdated Show resolved Hide resolved
@AntoineAugusti AntoineAugusti merged commit 8ee3371 into master May 15, 2020
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