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

Remove included calendars from gem #54

Merged
merged 7 commits into from
May 26, 2020
Merged

Conversation

JoeSouthan
Copy link
Contributor

@JoeSouthan JoeSouthan commented May 4, 2020

Why?

  • We now have 2 different packages for managing business date calculations: gocardless/business-python, keeping the data up to date between them will prove more and more difficult
  • The vast majority of users do not need all of the included calendars

Migration

If you currently use any of the bundled calendars, please copy them to a folder within your project root - Existing calendar data can be found here

For example:

Before:

Business::Calendar.load("bacs")

After 2.0

Business::Calendar.load_paths("lib/calendars") # lib/calendars contains bacs.yml
Business::Calendar.load("bacs")

@JoeSouthan JoeSouthan force-pushed the joesouthan/remove-calendars branch 2 times, most recently from 41d74bd to 129e148 Compare May 4, 2020 15:58
@JoeSouthan JoeSouthan marked this pull request as ready for review May 4, 2020 15:59
CHANGELOG.md Outdated

**BREAKING CHANGES** 🚨

- Remove bundled calendars see [this pr](https://github.com/gocardless/business/pull/54)
Copy link
Contributor

@ahjmorton ahjmorton May 4, 2020

Choose a reason for hiding this comment

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

I'd add a second sentence here pointing to the old data in Github as a set of files.

Potentially a second in the README along the lines of "Hey, how do I unbreak now you've unbundled the data?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe with a message along the lines of "this data only goes until 2020, after that you're on your own"

@JoeSouthan JoeSouthan force-pushed the joesouthan/remove-calendars branch from 129e148 to 6d63112 Compare May 4, 2020 23:40
Copy link

@dchambersgc dchambersgc left a comment

Choose a reason for hiding this comment

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

The changes to remove the inbuilt directories are good.

The README needs to be looked at again, theres a few spelling mistakes and the flow isn't the smoothest for a reader. We could simplify the flow if we talk about using the calendar in one place, currently we mention it twice in slightly different ways, we also have a new change included in v2 half way through the README after we have talked about the other changes in v2 at the top.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@JoeSouthan JoeSouthan force-pushed the joesouthan/remove-calendars branch 2 times, most recently from 53150bb to c3ad96c Compare May 5, 2020 09:38
CHANGELOG.md Show resolved Hide resolved
Copy link

@dchambersgc dchambersgc left a comment

Choose a reason for hiding this comment

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

Some further thoughts. It's reading a lot better so thank you for that.

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link

@dchambersgc dchambersgc left a comment

Choose a reason for hiding this comment

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

Some smaller suggestions, it's coming together nicely.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dchambersgc
Copy link

@JoeSouthan One thing to be aware of, we have typo'd the gem api in the PR description. It should read Business::Calendar instead of Calendar::Business

@JoeSouthan JoeSouthan force-pushed the joesouthan/remove-calendars branch from a5d0545 to 4bac75e Compare May 7, 2020 14:19
@JoeSouthan
Copy link
Contributor Author

@dchambersgc Whoops! corrected the pr and the readme

@JoeSouthan JoeSouthan force-pushed the joesouthan/remove-calendars branch from 4bac75e to 5e2aaef Compare May 7, 2020 14:20
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