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

[historical] throw error if given a string for period{1,2} that is invalid input for new Date() #460

Merged
merged 3 commits into from
May 1, 2022

Conversation

glaucoheitor
Copy link
Contributor

@glaucoheitor glaucoheitor commented Apr 29, 2022

Closes #457 .

Changes

Type

  • New Module
  • Other New Feature
  • Validation Fix
  • Other Bugfix
  • Docs
  • Chore/other

Comments/notes

First time submitting a 'real' pull request with code. I didn't add tests because I'm not familiar with it.
Also, I'm not 100% sure if my change is the best decision for this issue, but I try a few different options and I liked this the most. Please let me know if I can improve this further and if I made something wrong so I can improve next time. Thanks for the amazing project, as always! :)

@codecov
Copy link

codecov bot commented Apr 29, 2022

Codecov Report

Merging #460 (10ac8cc) into devel (0a15106) will decrease coverage by 0.19%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##            devel     #460      +/-   ##
==========================================
- Coverage   97.92%   97.73%   -0.20%     
==========================================
  Files          22       22              
  Lines         482      485       +3     
  Branches      158      159       +1     
==========================================
+ Hits          472      474       +2     
- Misses         10       11       +1     
Impacted Files Coverage Δ
src/modules/historical.ts 97.14% <80.00%> (-2.86%) ⬇️

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 0a15106...10ac8cc. Read the comment docs.

@gadicc
Copy link
Owner

gadicc commented Apr 30, 2022

Hey @glaucoheitor, awesome work! Thanks for stepping up 🙌 It's really great for maintainers when users are contributing back and this is greatly appreciated 😁

Yes, I was also thinking of using isNaN, so we're on the same page. And throwing an error exactly as you have.

I have two minor comments which I'll place inline. And no problem re the test, I'll do that after we merge and I think it will make a lot of sense to you once you see it :D

Thanks again!

src/modules/historical.ts Outdated Show resolved Hide resolved
src/modules/historical.ts Show resolved Hide resolved
@glaucoheitor
Copy link
Contributor Author

I'm glad to help @gadicc. I really like this project and makes me happy to be able to help just a lil bit. Thanks for the opportunity and your patience.

@gadicc gadicc merged commit 58b98d9 into gadicc:devel May 1, 2022
@gadicc
Copy link
Owner

gadicc commented May 1, 2022

Awesome, thanks @glaucoheitor! For sure, and all help is super appreciated, so thanks again!

This will be in the next release. There'll be an automated message here when it gets published. I'll also tag this PR when I add the test for the new code path.

🎉🙏

@gadicc
Copy link
Owner

gadicc commented May 1, 2022

@glaucoheitor, quoting from above commit:

describe("historical", () => {
  // ...

  it("throws if period{1,2} gets an invalid string for new Date()", async () => {
    await expect(yf.historical("TSLA", { period1: "invalid" })).rejects.toThrow(
      /invalid date provided/
    );

    await expect(
      yf.historical("TSLA", { period1: "2022-02-022", period2: "invalid" })
    ).rejects.toThrow(/invalid date provided/);
  });

  //...
});

So, we:

  1. describe a test suite called "historical".
  2. have a test to check if it (historical) "throws if period{1,2} is invalid"
  3. expect the yf.historical() command to reject (since it's a promise) and toThrow (an error), which we can match with a regexp (against the error message)

Hope that helps :) If you want to familiarize yourself further, you pull in the latest devel (with the tests above), run yarn test --watch src/modules/historical.spec.ts and see what happens when you play with the test a little bit (you could e.g. give a valid string so the command won't throw, which will cause the test (which is expecting it to throw) to fail (because now it's not throwing). Hope that makes sense!

@gadicc
Copy link
Owner

gadicc commented May 2, 2022

🎉 This PR is included in version 2.3.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gadicc gadicc added the released label May 2, 2022
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.

[historical] throw error if given a string for period{1,2} that is invalid input for new Date()
2 participants