Skip to content

Remove incorrect checks for PMT and FV calculations #67

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

Merged
merged 1 commit into from
May 10, 2022

Conversation

evanknapke
Copy link

@evanknapke evanknapke commented May 10, 2022

It is perfectly valid for FV and PV to both be zero in the PMT calculations.
It is perfectly valid for PMT and PV to both be zero in the FV calculations.

I have only fixed these, but it seems there are some other unnecessary checks like this in some of the other calculations that would be worth looking over.

@jcoliz
Copy link
Member

jcoliz commented May 10, 2022

Thanks for submitting this. I will have a look!

@jcoliz
Copy link
Member

jcoliz commented May 10, 2022

Looks good. Just waiting for the long-running "console" tests to finish. Also, I like for new code to have dedicated tests in spottests.fs, so I'll add those also.

@evanknapke Question for you... Why? Is there every any practical reason to call these functions in the way described here? The result can only be $0. You're correct that Excel allows it, so the library should too. Still, I'm curious what led you here?

Finally, do you need new packages released to NuGet Gallery? I can do so if you need it.

@jcoliz jcoliz merged commit 74675ed into fsprojects:master May 10, 2022
jcoliz added a commit that referenced this pull request May 10, 2022
@jcoliz jcoliz mentioned this pull request May 10, 2022
@evanknapke
Copy link
Author

Looks good. Just waiting for the long-running "console" tests to finish. Also, I like for new code to have dedicated tests in spottests.fs, so I'll add those also.

@evanknapke Question for you... Why? Is there every any practical reason to call these functions in the way described here? The result can only be $0. You're correct that Excel allows it, so the library should too. Still, I'm curious what led you here?

Finally, do you need new packages released to NuGet Gallery? I can do so if you need it.

@jcoliz Sure thing... We are using this library to run several calculations based off of a lot of user input. Sometimes the user may input data that leads to this part of the calculations to have 0 values for pmt, pv, and/or fv. If this is the case, exceptions were thrown and broke these parts of the process. We specifically noticed it in the FV and PMT functions, which is why these were the only ones I fixed in this Pull Request.

So, I agree that it does not make much sense to call these functions since it will always be $0, but it is still valid to do so and lead me to make the PR.

It would be great if you could make a release to NuGet if possible with these changes!

@jcoliz
Copy link
Member

jcoliz commented May 11, 2022

@evanknapke Sure thing. I have published it to the NuGet Gallery. It's indexing now, will be available soon. Please feel free to submit PRs for any other similar problems. Bonus points for checking in a failing unit test first (e.g. 20ff52e) in a separate commit.

Thanks for your contribution!

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

Successfully merging this pull request may close these issues.

2 participants