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

Sync workalendar v5.2.2 #9

Merged
merged 137 commits into from
Oct 27, 2019
Merged

Sync workalendar v5.2.2 #9

merged 137 commits into from
Oct 27, 2019

Conversation

ShaheedHaque
Copy link
Collaborator

@ShaheedHaque ShaheedHaque commented Jul 10, 2019

This relates to #8. This is not yet ready to merge, see notes below regarding tests and files which need specific review.

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

I've performed the merge, but my packaging skills are not up to getting the tests running (either tox or make test). I am able to:

  • sudo make install
  • Run a basic python3 test by hand:
$ python3
Python 3.7.3 (default, Apr  3 2019, 05:39:12) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from calendra.europe import united_kingdom
>>> foo = united_kingdom.UnitedKingdom()#.get_calendar_holidays(2020)
>>> foo.get_early_may_bank_holiday(2020)
Holiday(2020, 5, 8)

I would appreciate help getting the tests/tox to run. Also, the following files deserve closer review:

  • travis.yml is unchanged because I don't know Travis.
  • Contributing.* because I added a section on syncing.
  • CHANGES.rst because I added all the revision history.
  • tox.ini is unchanged because I don't know tox.
  • calendra/united_kindom.py because this was by far the most complicated merge.
  • calendra/usa/core.py because this was the second significant merge.
  • registry.py because I had to set load_standard_modules=False, and keep the old imports. I was not able to reconcile how to make the paths/imports work.

brunobord and others added 30 commits December 6, 2018 15:16
- Travis job configuration was upgraded to Xenial (16.04)
- ``tox.ini``: conditional requirement. We'll be using the last version of ``pandas`` compatible with Python 3.4, and the latest for other Python versions
Bugfix: Resolve incompatibilities between latest Pandas and Python 3.4
Missing to include as holiday the day before good_friday
based on the work of @reichert in workalendar#268.

Changes:

* Added tests for Paraguay class,
* Added the date variation for Heroes Day, Founding of Asunción and Boqueron Battle Victory Day in 2017,
* Added erroneous Independance day set to May 15th, wikipedia doesn't mention it,
* Removed New Years Eve, wikipedia doesn't mention it,
* Fixed flake8 issues,
* Added Paraguay to the list on the README file,
* Added changelog entry.
…n-pr-template

Added instructions about the usage of `iso_register` in the PR template
Holidays which are not included:

- There are holidays which are taken as day off by large portions of the
pupulation but are not considered official holidays, e.g. Chanuka.
- There are a lot of jewish holidays which are not public holidays
- There are muslim holidays which are conisdered days off for the Muslim
population
- Holidays which are optional paid leave e.g. Jerusalem Day
- School holidays
- Solved the incompatibility between `pandas` latest version and Python
  3.4. Upgraded travis distro to Xenial/16.04 LTS (workalendar#307).
- Added instructions about the usage of the `iso_register` decorator in
  the pull-request template (workalendar#309).

**New Calendars**

- Added New Zealand, by @johnguant (workalendar#306).
- Added Paraguay calendar, following the work of @reichert (workalendar#268).
- Added China calendar, by @iamsk (workalendar#304).
- Added Israel, by @armona, @tsehori (workalendar#281).
Fix a small flake8 issue with wrong indentation.
refs workalendar#31

* Scotland base calendars
* Major towns and cities have their own calendars

Four years after the beginning, I'm back.
**New calendars**

*WARNING* Scotland (sub)calendars are highly experimental and because of their very puzzling rules, may be false. Please use them with care.

- Added Scotland calendars, i.e. Scotland, Aberdeen, Angus, Arbroath, Ayr, Carnoustie & Monifieth, Clydebank, Dumfries & Galloway, Dundee, East Dunbartonshire, Edinburgh, Elgin, Falkirk, Fife, Galashiels, Glasgow, Hawick, Inverclyde, Inverness, Kilmarnock, Lochaber, Monifieth, North Lanarkshire, Paisley, Perth, Scottish Borders, South Lanarkshire, Stirling, and West Dunbartonshire (workalendar#31).

**Bugfixes**

- Fixed United Kingdom bank holiday for 2002 and 2012, thx @ludsoft (workalendar#315).
- Fix a small flake8 issue with wrong indentation (workalendar#319).
- Fix Russia "Day of Unity" date, set to November 4th, thx @alexitkes for the bug report (workalendar#317).
@ShaheedHaque
Copy link
Collaborator Author

ShaheedHaque commented Jul 11, 2019

Pytest and tox now run as follows for me:

==== 2305 passed, 158 skipped, 14 warnings in 12.47 seconds =====

But I still have no clue about travis. I think I will have to leave that to you @jaraco! Also, in addition to the list of "things you might want to look at" in my previous comment, ca90ebb might be worth review.

@ShaheedHaque
Copy link
Collaborator Author

OK, now that Travis is working, I think the code is ready to merge subject to your review @jaraco. To summarise, the current areas recommended for specific attention are:

  • Contributing.* because I added a section on syncing.
  • CHANGES.rst because I added all the revision history.
  • calendra/united_kindom.py because this was by far the most complicated merge.
  • calendra/usa/core.py because this was the second significant merge.
  • registry.py because I had to set load_standard_modules=False, and keep the old imports. I was not able to reconcile how to make the paths/imports work.
  • a90ebb because it contains specific "design" changes to the respective tests.

@ShaheedHaque
Copy link
Collaborator Author

Ping...any prospect of getting this closed out before upstream drifts too far?

Copy link
Owner

@jaraco jaraco left a comment

Choose a reason for hiding this comment

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

All things considered, this is looking great. The main issue is the use of 'shift' functions that create duplicate holidays. If we can eliminate those, I'll feel really good about accepting this. Everything else is mostly nitpicks. Let me know if you need/want help eliminating the shift functions.

calendra/europe/united_kingdom.py Outdated Show resolved Hide resolved
calendra/europe/united_kingdom.py Outdated Show resolved Hide resolved
spring_bank_holiday = date(2002, 6, 4)
else:
spring_bank_holiday = UnitedKingdom. \
get_last_weekday_in_month(year, 5, MON)
Copy link
Owner

Choose a reason for hiding this comment

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

Again, better to prefer relativedelta.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As above.

calendra/europe/united_kingdom.py Outdated Show resolved Hide resolved
calendra/europe/united_kingdom.py Outdated Show resolved Hide resolved
CHANGES.rst Show resolved Hide resolved
CONTRIBUTING.rst Outdated Show resolved Hide resolved
@@ -567,6 +568,23 @@ def get_whit_sunday(self, year):
def get_corpus_christi(self, year):
return self.get_easter_sunday(year) + timedelta(days=60)

def shift_christmas_boxing_days(self, year):
Copy link
Owner

Choose a reason for hiding this comment

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

I'm pretty sure this isn't needed.

calendra/usa/core.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@ShaheedHaque
Copy link
Collaborator Author

@jaraco just in case, I am hoping for further feedback from you on a few questions...at your convenience of source!

@ShaheedHaque
Copy link
Collaborator Author

Gentle ping...

@jaraco
Copy link
Owner

jaraco commented Oct 27, 2019

Thanks again Shaheed for the hard work putting this merge together. Please review my last few commits to see what I had in mind. I'll get this release out ASAP.

@jaraco jaraco merged commit b97f2ec into jaraco:master Oct 27, 2019
Copy link
Collaborator Author

@ShaheedHaque ShaheedHaque left a comment

Choose a reason for hiding this comment

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

I see what you mean about relativedelta; it does indeed make the code quite a bit nicer.

Copy link
Collaborator Author

@ShaheedHaque ShaheedHaque left a comment

Choose a reason for hiding this comment

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

I'm not qualified to comment as I find Python packaging baroque and impenetrable beyond reasonableness ;-).

@ShaheedHaque
Copy link
Collaborator Author

ShaheedHaque commented Oct 27, 2019 via email

@jaraco
Copy link
Owner

jaraco commented Oct 27, 2019

I expect the 4.0.0 release to go out here.

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.

None yet