Skip to content

Commit

Permalink
[issue-24] Reorganize documentation into doc/ and add ADR for custom …
Browse files Browse the repository at this point in the history
…methods change
  • Loading branch information
Phil Peble committed Sep 30, 2018
1 parent 8e89255 commit 90402fa
Show file tree
Hide file tree
Showing 7 changed files with 127 additions and 31 deletions.
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
# Holiday definitions

## 3.0.0

Major semver bump as the format for custom methods has been changed to complete [issue-24](https://github.com/holidays/definitions/issues/24). Downstream consumers will need to update to be able to parse them. However there are **no behavior changes** with this update.

In summary: we have switched to language-specific custom methods. Instead of a plain `source` field you will need a specific language implementation, e.g. `ruby`, `golang`, etc.

Currently we only have `ruby` but we can now expand these definitions for use in other languages. Please see the [custom methods ADR](doc/architecture/adr-001.md) for more in-depth information on why this change was made.

You can also view the updated ['Methods' section in the SYNTAX doc](doc/SYNTAX.md#methods) for more info and examples.

## 2.5.3

* Add missing `observed` logic for 'St. Patricks Day' in `gb_nir`
Expand Down
14 changes: 6 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,15 @@ Currently it is only used by the [existing Holidays gem](https://github.com/holi
definitions and generates ruby classes for use in that gem. In the future it will be used by other languages in
a similar manner.

*Please note* that this is *not* a gem. The validation process is written in ruby simply for convenience. The real
**Please note** that this is _not_ a gem. The validation process is written in ruby simply for convenience. The real
stars of this show are the YAML files.

### Syntax
### Documentation

The definition syntax is a custom format developed over the life of this project. Please see
[our syntax doc](SYNTAX.md) for more information on how to format definitions.

### How to contribute

See our [contribution guidelines](CONTRIBUTING.md) for information on how to help out!
1. [Syntax Guide](doc/SYNTAX.md)
2. [Contribution Guidelines](doc/CONTRIBUTING.md)
3. [Maintainer Guidelines](doc/MAINTAINERS.md)
4. [Architecture Decision Records](doc/architecture/README.md)

### Credits

Expand Down
18 changes: 6 additions & 12 deletions CONTRIBUTING.md → doc/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ In this repository we have all of the definitions that are used in holiday calcu

Please read our [Code of Conduct](https://github.com/holidays/holidays/blob/master/CODE_OF_CONDUCT.md) before contributing. Everyone interacting with this project (or associated projects) is expected to abide by its terms.

## For definition updates
## Definition Updates

Our definitions are written in YAML. You can find a complete guide to our format in the [README](README.md). We take the YAML definitions and generate final definition files in the various projects
that are loaded at runtime for fast calculations.
Our definitions are written in YAML. You can find a complete guide to our format in the [syntax docs](SYNTAX.md). We take the YAML definitions and generate final definition files in the various projects that are loaded at runtime for fast calculations.

Here are the steps to take once you have a good idea on what you want to change:

Expand All @@ -18,8 +17,7 @@ Here are the steps to take once you have a good idea on what you want to change:
* Run `make validate` to ensure that all updates match our definition format
* Open a PR with your changes

Including documentation with your updates is very much appreciated. A simple Wikipedia entry or government link in the
comments alongside your changes would be perfect.
Including documentation with your updates is very much appreciated. A simple Wikipedia entry or government link in the comments alongside your changes would be perfect.

Lastly, note that there are many 'meta' regions. For example, there are regions for Europe, Scandinavia, and North America. If your new region(s) falls into these areas consider adding them. You can find these 'meta' regions in `definitions/index.yaml`.

Expand All @@ -29,12 +27,8 @@ Don't worry about versioning, we'll handle it on our end.

## Definition Validation

We maintain a `make validate` command to ensure that all YAML definitions match our internal specifications. This is to make
working with this repository as independent as possible from the other repositories (like the existing ruby repository). If
`make validate` passes then we ensure that anything consuming these files will receive 'correct' formats.
We maintain a `make validate` command to ensure that all YAML definitions match our internal specifications. This is to make working with this repository as independent as possible from the other repositories (like the existing ruby repository). If `make validate` passes then we ensure that anything consuming these files will receive 'correct' formats.

If you run into any weird `make validate` errors please open an issue or PR and highlight to what you are seeing. The
validation code is brand-new and might have issues. Maintainers will respond quickly to any open problems.
If you run into any weird `make validate` errors please open an issue or PR and highlight to what you are seeing. The validation code is brand-new and might have issues. Maintainers will respond quickly to any open problems.

If you would like to add to, update, or otherwise fix any of our specs then please fork and submit a PR like you would any
other change. Please note that we require 100% test coverage. Your builds will not pass if you fall below 100%.
If you would like to add to, update, or otherwise fix any of our specs then please fork and submit a PR like you would any other change. Please note that we require 100% test coverage. Your builds will not pass if you fall below 100%.
12 changes: 3 additions & 9 deletions MAINTAINERS.md → doc/MAINTAINERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,8 @@ will need to investigate further (contact a core member for assistance).
that has the new version and associated changes. This is pretty open-ended! Include the information that you feel is
important. Use past CHANGELOG updates as a guide.
* Open a PR against the CHANGELOG branch and merge it (this may require another maintainer for safety)
* Once the updated CHANGELOG is merged, go to the [releases](https://github.com/holidays/definitions/blob/master/CHANGELOG.md) page
and create a new release. The release should point at the latest commit that contains the changes that you want included in
this release. If you just merged then you can just point at master. All release versions follow this
format: `vMAJOR.MINOR.PATCH`. This follows the normal [semver](https://github.com/holidays/definitions/blob/master/CHANGELOG.md)
rules. Look at recent releases to figure out what the new version should be.
* Once the updated CHANGELOG is merged, go to [releases](https://github.com/holidays/definitions/releases) and create a new release. It should point at the latest commit that contains the changes that you want included in this release. If you just merged then you can just point at master. All release versions follow this format: `vMAJOR.MINOR.PATCH`. This should follow normal [semver rules](https://semver.org/).

You don't need to list out the specific changes that were made on the release, you can just give a general overview. You can link to the
updated CHANGELOG that you did in a previous step. Example: [v2.2.0](https://github.com/holidays/definitions/releases/tag/v2.2.0)
You don't need to list out the specific changes that were made on the release description. You can just give a general overview and then link to the updated CHANGELOG that you did in a previous step. Example: [v2.2.0](https://github.com/holidays/definitions/releases/tag/v2.2.0)

Once the release is created in Github you are done! The definitions have been 'released' and downstream projects (right now just ruby)
can reference them without issues. See the downstream maintainers guides for information on how to release updates.
Once the release is created in Github you are done! The definitions have been 'released' and downstream projects (right now just ruby) can reference them without issues. See the maintainers guides in downstream projects for information on how to release updates for each language.
4 changes: 2 additions & 2 deletions SYNTAX.md → doc/SYNTAX.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Holiday Gem Definition Syntax
# Holiday Definition Syntax

All holidays are defined in these YAML files. These definition files have three main top-level properties:
The definition syntax is a custom format developed over the life of this project. All holidays are defined in these YAML files. These definition files have three main top-level properties:

* `months` - this is the meat! All definitions for months 1-12 are defined here
* `methods` - this contains any custom logic that your definitions require
Expand Down
14 changes: 14 additions & 0 deletions doc/architecture/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# Architecture Decision Records

Here we document decisions we made regarding high level architecture/design of this repository.

For details on what ADR is and why it's important, please reference the following blog posts:

- https://product.reverb.com/documenting-architecture-decisions-the-reverb-way-a3563bb24bd0
- http://thinkrelevance.com/blog/2011/11/15/documenting-architecture-decisions

Please note that we only began keeping ADRs for this project in October of 2018. Decisions made before that are not covered.

## Table of contents

1. [Language specific custom methods](adr-001.md)
86 changes: 86 additions & 0 deletions doc/architecture/adr-001.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# ADR 1: Custom Methods Format Change

## Context

We would like these definitions to be usable by any language. The original `holidays` project was written purely in `ruby` but the definitions were generally plain `YAML`.

The issue is that `ruby` has been sprinkled into the otherwise plain `YAML` when it made sense with no plan for use outside of `ruby`. This makes sense when you are never planning on using the `YAML` files in other languages.

Over time we have [been working](https://github.com/holidays/definitions/issues/7) to make the syntax more generic so that other language implementations could consume them. The last hurdle was custom methods.

An example of the original format:

```yaml
methods:
ca_victoria_day:
arguments: year
source: |
date = Date.civil(year, 5, 24)
if date.wday > 1
date -= (date.wday - 1)
elsif date.wday == 0
date -= 6
end
date
```
As you can see the actual function is just plain `ruby`.

After lots of trial and error I have decided that I cannot see a generic format for this logic that would satisfy all use cases for existing custom methods in our definitions. While some custom methods are relatively simple `if/else` statements there are many that are much more complicated.

An example of a 'complicated' custom method from the `ch` (Swiss) region:

```yaml
ch_vd_lundi_du_jeune_federal:
# Monday after the third Sunday of September
arguments: year
ruby: |
date = Date.civil(year,9,1)
# Find the first Sunday of September
until date.wday.eql? 0 do
date += 1
end
# There are 15 days between the first Sunday
# and the Monday after the third Sunday
date + 15
```

The logic itself is not hard to follow but coming up with a generic way to phrase this seems like a complex problem. Every attempt that was made devolved into very complex parsers of nested `YAML` so that we correctly handled each new edge case that appeared. It was very slow going and the complexity was growing and growing.

Additionally, having a complex `YAML` syntax for custom methods would require each downstream repository to implement the 'standard'. That seems pretty scary to think about maintaining.

The other option is to just make each future language provide their own implementations.

## Decision

The decision is to simply require language-specific implementations of custom methods. Since all custom methods are currently in `ruby` we are changing every `source` field to `ruby`. In the future new languages will need to provide their own implementations. For example, we could add a `golang` or `swift` section next to the existing `ruby` section.

There are three significant advantages:

- It is very easy to understand
- All holidays using custom methods have tests so each downstream project will have built-in protection in case a bug is introduced in only one language implementation
- It is very easy to implement for the current `ruby` implementation (which is our only project currently)

There are significant downsides:

- Possible divergence between languages due to separate implementations, causing confusion and frustration
- More pressure on maintainers to handle the various implementations, ensuring they can build without issues when new custom methods are added
- Confusion for new contributors who may only be comfortable in a single language
- New downstream languages will have a higher hurdle to overcome since they will need to implement the existing logic in their own language

In the end I don't want to hold things up because of _possible_ new language implementations that might show up in the future. I personally want to create a new `golang` version of `holidays` but beyond that maybe no one else will consume these definitions!

If the `holidays` projects become wildly popular in the future and this becomes a huge problem then I can address it with the (presumably huge) community to find a solution.

## Consequences

We might lose contributions due to confusion or fear.

We might burn out maintainers if the juggling of languages becomes too much of a burden.

This puts more pressure for the completion of an updated [test framework](https://github.com/holidays/definitions/issues/42) for downstream repositories.

## Status

Accepted.

0 comments on commit 90402fa

Please sign in to comment.