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

chore: enable strictBindCallApply #2254

Merged
merged 12 commits into from
Jul 18, 2023
Merged

chore: enable strictBindCallApply #2254

merged 12 commits into from
Jul 18, 2023

Conversation

Shinigami92
Copy link
Member

I experiment and moved the repeating code into an internal function.
It seems to work and we can really pass this just down into the function.

However typing this function correctly exceeds my TypeScript knowledge 😖

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jul 15, 2023
@Shinigami92 Shinigami92 self-assigned this Jul 15, 2023
@codecov
Copy link

codecov bot commented Jul 15, 2023

Codecov Report

Merging #2254 (cdc49b6) into next (a3a1480) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff            @@
##             next    #2254    +/-   ##
========================================
  Coverage   99.59%   99.60%            
========================================
  Files        2637     2638     +1     
  Lines      244872   244684   -188     
  Branches     1157     1084    -73     
========================================
- Hits       243890   243707   -183     
+ Misses        955      950     -5     
  Partials       27       27            
Impacted Files Coverage Δ
src/internal/bind-this-to-member-functions.ts 100.00% <100.00%> (ø)
src/modules/airline/index.ts 100.00% <100.00%> (ø)
src/modules/animal/index.ts 100.00% <100.00%> (ø)
src/modules/color/index.ts 99.79% <100.00%> (-0.01%) ⬇️
src/modules/commerce/index.ts 100.00% <100.00%> (ø)
src/modules/company/index.ts 100.00% <100.00%> (ø)
src/modules/database/index.ts 100.00% <100.00%> (ø)
src/modules/datatype/index.ts 100.00% <100.00%> (ø)
src/modules/date/index.ts 100.00% <100.00%> (ø)
src/modules/finance/index.ts 100.00% <100.00%> (ø)
... and 17 more

... and 1 file with indirect coverage changes

@Shinigami92
Copy link
Member Author

I asked ChatGPT for some help

However I used GitHub Copilot to help me a bit to define the types

I don't really understand what's going on, or to be more precise: "why is this working" 🤔 🤷

@Shinigami92
Copy link
Member Author

@ST-DDT and @xDivisionByZerox

please check if you are both

  1. satisfied with the name of the function
  2. satisfied with the order of the params, or we should swap them ((that, moduleClass))
  3. want to change the JSDocs and maybe bring the inline comment up to the JSDoc

Additionally I think I should add a unit test that explicitly checks that the function is doing for what it is intended
But before I do that, please confirm that I'm on the right track and should move on with this

@xDivisionByZerox
Copy link
Member

xDivisionByZerox commented Jul 15, 2023

  1. satisfied with the name of the function

I'd probably go more for something like bindThisScope or bindThisForFunctions, since namespace is a different concept in JS.

  1. satisfied with the order of the params, or we should swap them ((that, moduleClass))

See my review comment.

  1. want to change the JSDocs and maybe bring the inline comment up to the JSDoc

I'll have an additional review for that most likely.

Additionally I think I should add a unit test that explicitly checks that the function is doing for what it is intended

Big +1. Working with this scopes can get quite confusing for some people. So, a test should help verify the intentions of the function.

@Shinigami92
Copy link
Member Author

Great help 🙌 Thank you both 😻

@Shinigami92
Copy link
Member Author

LOL Today I learned:

⎯⎯⎯⎯⎯⎯⎯ Failed Tests 1 ⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/internal/bind-this-to-member-functions.spec.ts > internal > bind-this-to-member-functions > should bind this to member functions
AssertionError: expected error to have message: Cannot read properties of undefined (reading 'faker')

- Expected
+ Received

- Cannot read properties of undefined (reading 'faker')
+ Cannot read property 'faker' of undefined

NodeJS v14 has another error message 😆

@Shinigami92 Shinigami92 marked this pull request as ready for review July 15, 2023 12:52
@Shinigami92 Shinigami92 requested a review from a team as a code owner July 15, 2023 12:52
@Shinigami92 Shinigami92 requested a review from ST-DDT July 17, 2023 19:02
@Shinigami92 Shinigami92 enabled auto-merge (squash) July 18, 2023 05:18
@Shinigami92 Shinigami92 merged commit 5f947cb into next Jul 18, 2023
@ST-DDT ST-DDT deleted the strictBindCallApply branch October 9, 2023 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants