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

Mark toHaveBeenCalledBefore and After's second parameter as optional #650

Merged
merged 2 commits into from
Oct 9, 2023

Conversation

Blond11516
Copy link
Contributor

@Blond11516 Blond11516 commented Sep 11, 2023

What

Marks toHaveBeenCalledBefore and toHaveBeenCalledAfter's second parameter as optional in the type declarations.

Why

Current type declarations generate an error if the parameter isn't passed. This is inaccurate, as the default value will be used in this case.

Notes

Fixes #651

Housekeeping

  • Unit tests
  • Documentation is up to date
  • No additional lint warnings
  • Typescript definitions are added/updated where relevant

`toHaveBeenCalledBefore`'s first parameter should be optional because it has a default value.
@changeset-bot
Copy link

changeset-bot bot commented Sep 11, 2023

⚠️ No Changeset found

Latest commit: ee69452

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@chornos13
Copy link

I think you forgot to mark the second argument of toHaveBeenCalledAfter function as an optional

@Blond11516 Blond11516 changed the title Mark toHaveBeenCalledBefore's second parameter as optional Mark toHaveBeenCalledBefore and After's second parameter as optional Oct 6, 2023
@Blond11516
Copy link
Contributor Author

@chornos13 I didn't personally need toHaveBeenCalledAfter when I opened the PR but I think it makes sense to do both at the same time. I've updated the PR.

@codecov
Copy link

codecov bot commented Oct 9, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (61df485) 100.00% compared to head (ee69452) 100.00%.
Report is 9 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #650   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           73        73           
  Lines          674       674           
  Branches       290       290           
=========================================
  Hits           674       674           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@keeganwitt keeganwitt merged commit 9231e42 into jest-community:main Oct 9, 2023
23 checks passed
@keeganwitt
Copy link
Collaborator

Thanks for the PR!

keeganwitt added a commit that referenced this pull request Oct 9, 2023
@SimenB
Copy link
Member

SimenB commented Oct 9, 2023

Should we add an action that checks if either there's an added changeset or we add a label? Seems the message from the bots is overlooked by contributors most of the time 😅

@keeganwitt
Copy link
Collaborator

keeganwitt commented Oct 9, 2023

Should we add an action that checks if either there's an added changeset or we add a label? Seems the message from the bots is overlooked by contributors most of the time 😅

I'm not too picky about how we do it. I'm fine with that approach.

I know some projects force a changeset for every PR. On the one hand, seems like maybe overkill, but on the other hand, I haven't seen a PR that would be awkward to have this rule.

@SimenB
Copy link
Member

SimenB commented Oct 9, 2023

Not all PRs need a changeset - e.g. docs only. But seeing as the comment is ignored, we might need something that yells louder before merging

@keeganwitt
Copy link
Collaborator

Not all PRs need a changeset - e.g. docs only. But seeing as the comment is ignored, we might need something that yells louder before merging

Oh, I thought docs used the changeset for publishing the updated docs to the website.

@SimenB
Copy link
Member

SimenB commented Oct 10, 2023

Nah, changesets is solely for publishing to npm.

Website changes are apparently a manual step (#648 (comment)) - ideally that would just be autopublished on pushes to main. That's how https://jestjs.io/ works

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