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

Add FiscalDay #13

Merged
merged 14 commits into from
Dec 14, 2020
Merged

Add FiscalDay #13

merged 14 commits into from
Dec 14, 2020

Conversation

nicmendoza
Copy link
Contributor

Hey @adamjstewart here's a possible implementation. Let me know what you think. I can add documentation if/when you feel good about the PR.

@codecov-io
Copy link

codecov-io commented Dec 11, 2020

Codecov Report

Merging #13 (44a8f6d) into master (c03bc22) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master       #13    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            1         1            
  Lines          364       485   +121     
==========================================
+ Hits           364       485   +121     
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 c03bc22...44a8f6d. Read the comment docs.

@adamjstewart
Copy link
Owner

Since there is overlap between this and your FiscalMonth PR, I'm going to focus on reviewing the other PR first.

@nicmendoza
Copy link
Contributor Author

Merged master, so should be good to review now.

fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
test_fiscalyear.py Outdated Show resolved Hide resolved
@nicmendoza
Copy link
Contributor Author

Added some simple documentation.

Does it make sense to add sections for FiscalMonth and FiscalDay to the readme? Not sure why the thought didn't occur to me with the last PR, but even if it's a bit redundant it might help someone identify that the library is relevant for their use case.

Also, I added very subtle documentation for the isleap attribute I needed to add to FiscalYear. I'm not sure anyone will ever find it terribly useful, but it felt weird not documenting it so I sort of snuck it into the end of the example for FiscalYear.

README.rst Outdated Show resolved Hide resolved
fiscalyear.py Outdated Show resolved Hide resolved
@adamjstewart
Copy link
Owner

Does it make sense to add sections for FiscalMonth and FiscalDay to the readme?

Yes, please add to the README.rst and basics.rst, either in this PR or a follow-up PR.

Also, I added very subtle documentation for the isleap attribute I needed to add to FiscalYear. I'm not sure anyone will ever find it terribly useful, but it felt weird not documenting it so I sort of snuck it into the end of the example for FiscalYear.

I think that's good. It will also appear in the Sphinx-generated API docs.

@adamjstewart adamjstewart merged commit 430fb74 into adamjstewart:master Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants