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

Implement groupBy, withResolvers method #1352

Merged
merged 3 commits into from
Jul 3, 2024
Merged

Conversation

ksh8281
Copy link
Contributor

@ksh8281 ksh8281 commented Jul 2, 2024

No description provided.

Signed-off-by: Seonghyun Kim <[email protected]>
Signed-off-by: Seonghyun Kim <[email protected]>
@ksh8281 ksh8281 changed the title Implement groupBy method Implement groupBy, withResolvers method Jul 2, 2024
Comment on lines 85 to 89
PromiseReaction::Capability PromiseObject::newPromiseCapability(ExecutionState& state, const Value& constructor, const Value& parentPromise)
{
if (!constructor.isConstructor()) {
ErrorObject::throwBuiltinError(state, ErrorCode::TypeError, ErrorObject::Messages::Not_Constructor);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using the existing PromiseObject::newPromiseCapability(ExecutionState& state, Object* constructor, const Value& parentPromise) function instead?
This function is invoked by only one function, and constructor check can be done on the caller side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Signed-off-by: Seonghyun Kim <[email protected]>
Copy link
Contributor

@clover2123 clover2123 left a comment

Choose a reason for hiding this comment

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

LGTM

@clover2123 clover2123 merged commit d9bfe96 into Samsung:master Jul 3, 2024
27 checks passed
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