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.

Sign up for GitHub

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

Jump to bottom

Add P.object and P.object.empty #233

Closed
wants to merge 4 commits into from
Closed

Add P.object and P.object.empty #233

wants to merge 4 commits into from

Conversation

gitsunmin
Copy link

@gitsunmin gitsunmin commented Mar 23, 2024 •
edited

Add P.object and P.object.empty

Add a pattern to match all 'Object' and 'Empty Object'.

P.object

const fn = (input: object) =>
  match(input)
    .with(P.object, () => 'Object!')
    .otherwise(() => '???');

console.log(fn({ str: 'hello' })); // Object!

P.object.empty

const fn = (input: string) =>
  match(input)
    .with(P.object.empty(), () => 'Empty!')
    .otherwise(() => 'Full!');

console.log(fn({})); // Empty!

Issue number: #230

@gitsunmin gitsunmin changed the title Add P.object and P.object.empty #230 Add P.object and P.object.empty Mar 23, 2024
gvergnaud
gvergnaud reviewed Mar 27, 2024
View reviewed changes
Copy link
Owner

@gvergnaud gvergnaud left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! I have a few comments but that's a good start :)

src/patterns.ts Outdated
pattern: pattern
): ObjectChainable<pattern> =>
Object.assign(chainable(pattern), {
empty: () => objectChainable(intersection(pattern, emptyObject())),
Copy link
Owner

Choose a reason for hiding this comment

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

Seems like chainable would be enough here, since you can't chain empty several times

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I'll change it to "chainable." If it's wrong, please "comment."

src/patterns.ts Outdated Show resolved Hide resolved
src/types/Pattern.ts Outdated
@@ -15,6 +15,7 @@ export type MatcherType =
| 'or'
| 'and'
| 'array'
| 'object'
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure a new pattern type is necessary here because both patterns you added are implemented with guards

Copy link
Author

Choose a reason for hiding this comment

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

I'll erase "object".

src/types/Pattern.ts Outdated
@@ -191,7 +194,6 @@ export type NullishPattern = Chainable<
GuardP<unknown, null | undefined>,
never
>;

Copy link
Owner

Choose a reason for hiding this comment

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

Could you remove this diff?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I'll remove this diff

src/types/Pattern.ts Outdated
* match(value)
* .with(P.object.empty(), () => 'empty object')
*/
empty<input>(): ObjectChainable<
Copy link
Owner

Choose a reason for hiding this comment

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

It should just be Chainable here as well

tests/object.test.ts
type t = Expect<Equal<typeof obj, { str: 'world' }>>;
return obj.str;
})
.with(P.object, (obj) => {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you add test covering how P.object behaves with more inputs:

  • Functions
  • Primitive values
  • Null

It should catch all values that are assignable to the object type, and type narrowing and exhaustive should both work

Copy link
Author

Choose a reason for hiding this comment

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

I've added test. If you need more, please comment

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, functions and arrays are part of the object type in TypeScript: Playground

I think P.object should catch anything assignable to object to stay close to typescript semantics and play nicely with exhaustiveness checking.

I'm going to update your PR shortly

@gitsunmin
Fix multiple issues based on PR review feedback
1831e29 
- Comment: Seems like chainable would be enough here, since you can't chain empty several times
- Comment: I think we could make this more efficient by using a for in loop instead of Object.keys and breaking the loop by returning false if an object own property is encountered.
- Comment: It should just be Chainable here as well
- Comment: I'm not sure a new pattern type is necessary here because both patterns you added are implemented with guards
- Comment: Could you add test covering how P.object behaves with more inputs: Functions, Primitive values, Null. It should catch all values that are assignable to the object type, and type narrowing and exhaustive should both work
- Comment: Could you remove this diff?
@gvergnaud
Copy link
Owner

I wanted to make a few changes and realized I can't push to your repository so I had to create a new branch that includes your changes. Closing this PR in favor of #234

@gvergnaud gvergnaud closed this Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gvergnaud gvergnaud gvergnaud left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@gitsunmin @gvergnaud