-
Notifications
You must be signed in to change notification settings - Fork 13
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 FiscalMonth #12
Add FiscalMonth #12
Conversation
Codecov Report
@@ Coverage Diff @@
## master #12 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 269 364 +95
=========================================
+ Hits 269 364 +95
Continue to review full report at Codecov.
|
My first impression is that this looks good. I may have some suggestions for PEP8 formatting changes, although I can do those myself before I cut the next release. I'll try to review this in more detail sometime this week, but just ping me if I forget. Are there any other features your immediate project is looking for? What kind of timeline would you like to see a new release come out in? |
There's a world where we might need .fiscal_day on FiscalDate/Time and FiscalDay objects in the future, but at the moment the month-oriented additions are what we need. Something in the next 2-3 weeks would be stellar, but I can work around it in the meantime so don't rush anything on my account. |
That's definitely doable! My goal is to get all of this worked out and update a few testing-related things in the 1-2 week timeframe. |
Up to you. If you need them, or might need them, go ahead and add them. If not, we can always add them in a future release.
Completely agree. I would like to be as consistent as possible.
Yeah, I think that's the whole point. We could extend the
Yeah, I think they differ too much. I don't think this package is big enough that a metapackage is needed to keep things consistent. I'm pretty OCD, so I think I can manage for now. Maybe in the future if we add FiscalWeek, FiscalDay, etc.
Yeah, it's a little difficult to keep track. But that's what unit tests are for. That's also why I rely on other existing classes to compute these things. |
@nicmendoza any progress on this? No rush from my end, I just don't want you to think this PR has been abandoned. |
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
@adamjstewart made the requested changes and fixed the code style issues. |
Thanks, I'll try to have a look at this as soon as I get a chance. I'm preparing for a conference on Wednesday, so it might have to wait until after then. |
@adamjstewart Merged your latest changes from master. Also, I'm going to start in on FiscalDay as I am going to need it in the near future. Cheers! |
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
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.
I think this is the final thing that needs changing and we're good to merge!
Hey @adamjstewart, this is just an initial pass for your consideration. No documentation changes yet. Following your suggestion, this is mostly just a copy of FiscalYear (with some FiscalQuarter thrown in).
Unfortunately this branch includes the changes from #11 as I didn't expect to be doing multiple things in parallel on this project.Questions/Thoughts: