Add FiscalMonth

[nicmendoza]

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:

• Should the FiscalQuarter class get m1, m2, and m3 properties? Seems symmetrical with FiscalYear.
• I don't have a suggestion, but the fact that some properties return integers while others return instances seems wrong
• Similarly, and I think this is a domain problem, the fact that the datetime-derived classes use the calendar year and month as inputs (and in repr and str) while the FiscalYear, FiscalQuarter, and FiscalMonth classes all use the fiscal numbers regularly trips me up. I guess that's why this library is useful.
• It almost feels like a metaclass would make sense, although some of the implementation details (and method names, maybe more importantly) differ from one class to another. Would probably have be coupled with some API changes (e.g. next vs next_* methods).
• I am still cross-eyed trying to grok the difference between converting months from calendar -> fiscal vs fiscal -> calendar, but my implementation seems to work (see FiscalMonth.start). I couldn't find a reference implementation, but happy to do something different there.

[codecov-io]

Codecov Report

Merging #12 (594c07e) into master (2d28995) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #12   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines          269       364   +95     
=========================================
+ Hits           269       364   +95     

Impacted Files	Coverage Δ
fiscalyear.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d28995...594c07e. Read the comment docs.

[adamjstewart]

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?

[nicmendoza]

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.

[adamjstewart]

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.

[adamjstewart]

Should the FiscalQuarter class get m1, m2, and m3 properties? Seems symmetrical with FiscalYear.

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.

I don't have a suggestion, but the fact that some properties return integers while others return instances seems wrong

Completely agree. I would like to be as consistent as possible. fiscalyear is still in "alpha", so I'm not super concerned with backwards compatibility. With that said, if we can maintain backwards compatibility for at least a couple releases with a DeprecationWarning in-between, that would be ideal.

Similarly, and I think this is a domain problem, the fact that the datetime-derived classes use the calendar year and month as inputs (and in __repr__ and __str__) while the FiscalYear, FiscalQuarter, and FiscalMonth classes all use the fiscal numbers regularly trips me up. I guess that's why this library is useful.

Yeah, I think that's the whole point. We could extend the __repr__ and __str__ to append the fiscal info I suppose. But let's save that for another issue/PR.

It almost feels like a metaclass would make sense, although some of the implementation details (and method names, maybe more importantly) differ from one class to another. Would probably have be coupled with some API changes (e.g. next vs next_* methods).

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.

I am still cross-eyed trying to grok the difference between converting months from calendar -> fiscal vs fiscal -> calendar, but my implementation seems to work (see FiscalMonth.start). I couldn't find a reference implementation, but happy to do something different there.

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.

[adamjstewart]

@nicmendoza any progress on this? No rush from my end, I just don't want you to think this PR has been abandoned.

[nicmendoza]

@adamjstewart made the requested changes and fixed the code style issues. I see that the tests are failing, but I won't have time to look for at least a couple days. Not sure if it's something you changed? Fixed the broken tests as well.

[adamjstewart]

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.

[nicmendoza]

@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!

[adamjstewart]

I think this is the final thing that needs changing and we're good to merge!