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

feat: bindAllMethods util function #1476

Merged
merged 2 commits into from
Sep 6, 2023
Merged

Conversation

bmingles
Copy link
Contributor

Util functions for binding all methods of an object to itself.
resolves #1474

@codecov
Copy link

codecov bot commented Aug 31, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (6ff27a6) 45.75% compared to head (9b94f19) 45.77%.
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1476      +/-   ##
==========================================
+ Coverage   45.75%   45.77%   +0.02%     
==========================================
  Files         515      517       +2     
  Lines       35073    35116      +43     
  Branches     8784     8792       +8     
==========================================
+ Hits        16048    16076      +28     
- Misses      18974    18989      +15     
  Partials       51       51              
Flag Coverage Δ
unit 45.77% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
packages/utils/src/ClassUtils.ts 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

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

@bmingles bmingles requested review from mattrunyon and mofojed August 31, 2023 19:57
@bmingles bmingles assigned bmingles and unassigned mofojed and mattrunyon Aug 31, 2023
Comment on lines 16 to 17
instance[methodName] as (...args: unknown[]) => unknown
).bind(instance) as T[typeof methodName];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you have to cast here anyway, I don't think the generic type is worth the added complexity

let current = instance;

// Traverse the prototype chain until we get to Object.prototype
while (current != null && current !== Object.prototype) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might not want to iterate all the way back through the prototype chain? Not sure if this could cause anything to break in a React class component if it were used in one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I had that thought as well. I could see parameterizing it with a flag or something

current = Object.getPrototypeOf(current);
}

return [...methodNames.keys()].sort();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was easier for testing, but I could just sort in the test I guess

@bmingles bmingles requested a review from mattrunyon September 6, 2023 17:35
@bmingles bmingles merged commit 0dab8d7 into deephaven:main Sep 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2023
@bmingles bmingles deleted the 1474-bind-all branch September 6, 2023 18:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bind all methods util
3 participants