-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add fiscal_month attribute to FiscalDateTime and FiscalDate #11
Conversation
@adamjstewart is it possible re-running the build could resolve this issue? If I understand the logs, it seems like there's a plugin version resolution conflict with pytest specifically on Python 3.5 but the build+test are succeeding otherwise. |
Added a minor commit to re-trigger the build ☝ |
Codecov Report
@@ Coverage Diff @@
## master #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 266 269 +3
=========================================
+ Hits 266 269 +3
Continue to review full report at Codecov.
|
Had to pin pytest and pytest-mock to old versions to maintain support for python 2 .7 (assuming that's still a requirement) |
Hi @nicmendoza, thanks for the contribution! First impression: I like it, and I think the way you have things implemented looks correct. I'll go over this in more detail over the next couple of days. With regards to the pytest issue: I think pinning the version is the right thing to do. We don't use any new features of PyTest anyway. I would like to continue to offer Python 2 support because a lot of people are stuck with Python 2, especially in industry where they can't update their own computers. I use a couple of supercomputing clusters that are still stuck on Python 2.6 unfortunately. I've thought about dropping support for Python 2, 3.0-3.4 so that I can add type hints, but so far there hasn't been any demand for it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor suggestions, otherwise LGTM!
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
This PR specifically adds a fiscal_month attribute (and does not add a correspondingFiscalMonth object). I can follow up with fiscal_week and fiscal_day if desired, but I really needed this functionality for my use case and I'm hoping this PR stands on its own. It's pretty simple, and I think the approach could be quickly adapted to those two additional calculations if/when desired.
Please let me know if the PR is missing anything.