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

Updating mirr docstring [issue 76] #86

Merged
merged 4 commits into from
Nov 30, 2023
Merged

Updating mirr docstring [issue 76] #86

merged 4 commits into from
Nov 30, 2023

Conversation

RVitalii
Copy link
Contributor

This pull request addresses issue #76 by enhancing the documentation for the mirr function.

I have updated the docstring to provide clearer explanations of the function's purpose, parameters, and usage. Additionally, I incorporated a more detailed MIRR formula, including the reinvestment rate, and provided illustrative examples to guide users in understanding how to use the function effectively.

@Kai-Striega
Copy link
Member

Hello @RVitalii,

Thanks for taking the time to make a contribution!

This looks good. There are just a few problems with the style guide. At the moment, the CI is failing because some lines are too long. Here is the output from the CI:

numpy_financial/_financial.py:940:89: E501 Line too long (105 > 88)
numpy_financial/_financial.py:964:89: E501 Line too long (128 > 88)
numpy_financial/_financial.py:967:89: E501 Line too long (99 > 88)

This means that on lines 940,964 and 967 you are exceeding the limit of 88 chars. Would you please shorten those lines and I'll take another look through it?

numpy_financial/_financial.py Outdated Show resolved Hide resolved
numpy_financial/_financial.py Outdated Show resolved Hide resolved
@RVitalii
Copy link
Contributor Author

RVitalii commented Nov 26, 2023

Hello,

I tried to update my forked-main branch from upstream and amend the original commit, but did a lot of mess instead. I didn't know that the iss76 branch is already connected to upstream, and everything I had to do is to update that branch.

Eventually, I managed to push a new commit with the proposed changes:

b04ad78

Please, let me know if I have to correct smth.

Regards,
V.R.

@Kai-Striega
Copy link
Member

There's still one check failing. See the Lint check in the CI

Here's the failing output:

ruff 0.1.6
numpy_financial/_financial.py:940:89: E501 Line too long (93 > 88)
Found 1 error.
Error: Process completed with exit code 1.

@Kai-Striega Kai-Striega added the documentation Improvements or additions to documentation label Nov 27, 2023
@RVitalii
Copy link
Contributor Author

Hello @Kai-Striega,

Updated. Here is the commit number: 4fa06ed

Regards,
V.R.

@Kai-Striega
Copy link
Member

@RVitalii thanks for contributing, this looks good now! Are there any last changes you would like to do before I merge it?

@RVitalii
Copy link
Contributor Author

@Kai-Striega, no I am fine. Thanks for support.

@Kai-Striega Kai-Striega merged commit f202ec3 into numpy:main Nov 30, 2023
13 checks passed
@Kai-Striega
Copy link
Member

Merged. Thanks again for taking the time to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants