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

Update singapore.py #282

Closed
wants to merge 1 commit into from
Closed

Update singapore.py #282

wants to merge 1 commit into from

Conversation

pvalenti
Copy link
Contributor

@pvalenti pvalenti commented Sep 14, 2018

Added DEEPAVALI holidays for 2019 and 2020

  • Tests with a significant number of years to be tested for your calendar.
  • Changelog amended with a mention describing your changes.

Added DEEPAVALI holidays for 2019 and 2020
Copy link
Member

@brunobord brunobord left a comment

Choose a reason for hiding this comment

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

too bad, there's just a tiny fix to do in order to have the green tests with your code.

workalendar/asia/singapore.py:50:33: E261 at least two spaces before inline comment

Also, I'd recommend you to have a look at the contributing document. Your PR is cool, but it lacks a few items to make it in master:

  • Add some tests for the years you've added.
  • A Changelog item describing the change. For example: "Added DEEPAVALI holidays for 2019 and 2020 in Singapore calendar"

@brunobord
Copy link
Member

I'm going to take this PR over. don't worry, it'll be harmless :o)

brunobord added a commit that referenced this pull request Dec 6, 2018
Only bugfixes:

- Added DEEPAVALI days for 2019 and 2020, thx @pvalenti (#282).
- Fixed Germany Reformation Day miscalculation. Some German states include Reformation Day since the "beginning" ; in 2017, all states included Reformation Day as a holiday (500th anniversary of the Reformation) ; starting of 2018, 4 states added Reformation Day (#295).
@brunobord
Copy link
Member

Fixed and released on v3.2.1 - happy upgrading!
https://pypi.org/project/workalendar/3.2.1/

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.

2 participants