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

test(invariant): add test code #789

Merged
merged 2 commits into from
Nov 10, 2024
Merged

test(invariant): add test code #789

merged 2 commits into from
Nov 10, 2024

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Nov 4, 2024

The first argument of the invariant function is named condition, and given the use case, it would be nice to define it as a boolean type, what do you think?

Is there a reason why the type is unknown?

I additionally added test code for type narrowing for the invariant function.

Copy link

vercel bot commented Nov 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-toolkit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 9, 2024 3:38pm

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (0feabb4) to head (7c8fa9e).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #789   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         305      305           
  Lines        2727     2727           
  Branches      800      800           
=======================================
  Hits         2703     2703           
  Misses         23       23           
  Partials        1        1           

@raon0211
Copy link
Collaborator

raon0211 commented Nov 7, 2024

Actually it is common practice to write code like if (!number) or if (obj) -- that's the reason why we have unknown here. So I would like to stick with the current code.

@ssi02014
Copy link
Contributor Author

ssi02014 commented Nov 7, 2024

@raon0211
I understand. The naming of condition can be confusing, but as you said, it is correct to keep the current code to allow different types of values.

  1. Have you ever thought about renaming the argument?
  2. What do you think of the test code added by this work?

@raon0211
Copy link
Collaborator

raon0211 commented Nov 9, 2024

Yeah, the test code will definitely help. In the meantime, I thought using if (condition) is pretty common in JavaScript, so I figured condition would be a good name. Do you have any thoughts on the name?

@ssi02014
Copy link
Contributor Author

ssi02014 commented Nov 9, 2024

@raon0211 It would be nice to have a name that encompasses both, since it can take a value as an argument in addition to the usual condition, but unfortunately I don't have a good idea yet. 😭

So, I'm going to keep the existing code and just add the test code.

@ssi02014 ssi02014 changed the title fix(invariant): modify types and add test code test(invariant): add test code Nov 9, 2024
@ssi02014
Copy link
Contributor Author

ssi02014 commented Nov 9, 2024

@raon0211 I reverted the existing modifications, leaving only the test code. Thanks for the review. 🤗

Copy link
Collaborator

@raon0211 raon0211 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@raon0211 raon0211 merged commit 087a982 into toss:main Nov 10, 2024
8 checks passed
@ssi02014 ssi02014 deleted the imp/invariant branch November 10, 2024 07:41
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.

3 participants