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(linting): disable unbound-method warnings #1128

Merged
merged 2 commits into from
Jul 19, 2022

Conversation

xDivisionByZerox
Copy link
Member

This PR fixes nearly all linter warnings for the unbound-method rule.

@xDivisionByZerox xDivisionByZerox added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent labels Jul 4, 2022
@xDivisionByZerox xDivisionByZerox added this to the v7 - Current Major milestone Jul 4, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team July 4, 2022 15:29
@xDivisionByZerox xDivisionByZerox self-assigned this Jul 4, 2022
@xDivisionByZerox xDivisionByZerox requested a review from a team as a code owner July 4, 2022 15:29
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #1128 (2f29c4e) into main (f1cdab6) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1128   +/-   ##
=======================================
  Coverage   99.64%   99.65%           
=======================================
  Files        2146     2146           
  Lines      230401   230401           
  Branches      979      982    +3     
=======================================
+ Hits       229591   229603   +12     
+ Misses        789      777   -12     
  Partials       21       21           
Impacted Files Coverage Δ
src/modules/name/index.ts 100.00% <0.00%> (+1.08%) ⬆️
src/modules/internet/user-agent.ts 89.85% <0.00%> (+2.60%) ⬆️

@xDivisionByZerox
Copy link
Member Author

There would still be this:

faker/src/faker.ts

Lines 79 to 80 in b70d63d

readonly fake: Fake['fake'] = new Fake(this).fake;
readonly unique: Unique['unique'] = new Unique().unique;

I could call them in an anonym function as well but that would instantiate a new Fake or Unique every function call and I'm not happy with that.
I just noticed that these functions have no JSDocs when binding them like we do.

ST-DDT
ST-DDT previously approved these changes Jul 4, 2022
import-brain
import-brain previously approved these changes Jul 5, 2022
@ST-DDT
Copy link
Member

ST-DDT commented Jul 5, 2022

I could call them in an anonym function as well but that would instantiate a new Fake or Unique every function call and I'm not happy with that.

I currently see two ways for this:

  • convert them to standalone functions
  • use a private method to initialize the module instance and return the anonymous function to initialize the field with.

@Shinigami92
Copy link
Member

I could call them in an anonym function as well but that would instantiate a new Fake or Unique every function call and I'm not happy with that.

I currently see two ways for this:

  • convert them to standalone functions
  • use a private method to initialize the module instance and return the anonymous function to initialize the field with.

What is the general problem with how it currently works in main branch?
We already hacked it with the implementation in all the constructors using:

// Bind `this` so namespaced is working correctly
for (const name of Object.getOwnPropertyNames(<Module>.prototype)) {
  if (name === 'constructor' || typeof this[name] !== 'function') {
    continue;
  }
  this[name] = this[name].bind(this);
}

So if this PR just aims to get rid of the warning, we might just want to turn the warning off and don't care
Please also note that folks in real world already uses code like

import { faker } from '@faker-js/faker'

const uuid = faker.datatype.uuid

const id = uuid()

and until now, we support this
_even if I don't like our current hack, _ but the only way to really fix that, would be to rewrite EVERYTHING back to composable functions like

function datatype(faker = faker() /* or = new Faker() ?! */ ) {
    uuid() {
        return ... using something with faker. ....
    }
}

This would be a complete breaking change and currently I don't see that (at least not before we tackled more important parts like outsource locales / maybe using a monorepo)

@xDivisionByZerox
Copy link
Member Author

So if this PR just aims to get rid of the warning, we might just want to turn the warning off and don't care

So I'm reverting all changes and changing the linter config instead? Just give me a thumbs up or down and I will act accordingly. My only plan was to get rid of the warnings since they are triggering my OCD during development.

@ST-DDT
Copy link
Member

ST-DDT commented Jul 6, 2022

I'm fine either way, but the warnings bother me as well. Maybe put this on our agenda for tomorrow.

@xDivisionByZerox
Copy link
Member Author

Decision from todays meeting:
Remove the linter rule and revert the changes done in this PR. Explanation can be found in Shinigami's comment.

@xDivisionByZerox xDivisionByZerox force-pushed the chore/random/fix-linting-for-unbound-this branch from a4b78f2 to b70d63d Compare July 18, 2022 15:49
@xDivisionByZerox
Copy link
Member Author

I didn't close this PR :/ if you drop all changes you made, does it auto-close?

@Shinigami92
Copy link
Member

I didn't close this PR :/ if you drop all changes you made, does it auto-close?

Never heard of this behavior, in the log history of GitHub it says you closed it.

@xDivisionByZerox
Copy link
Member Author

xDivisionByZerox commented Jul 18, 2022

Never heard of this behavior, in the log history of GitHub it says you closed it.

I know, but I promise I didn't click the close button. Since we decided to not "fix" these warnings I first wanted to revert all commits I did and then just thought. Fk it, I'll rebase and drop all commits (basically resetting). After force pushing "the changes" I received a notification that I closed the PR ¯\(ツ)

@Shinigami92
Copy link
Member

I know, but I promise I didn't click the close button. Since we decided to not "fix" these warnings I first wanted to revert all commits I did and then just thought. Fk it, I'll rebase and drop all commits (basically resetting). After force pushing "the changes" I received a notification that I closed the PR ¯\(ツ)

RIP

@Shinigami92 Shinigami92 changed the title chore(linting): fix unbound-method warnings chore(linting): disable unbound-method warnings Jul 19, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) July 19, 2022 07:49
@Shinigami92 Shinigami92 merged commit ea91fe6 into main Jul 19, 2022
@Shinigami92 Shinigami92 deleted the chore/random/fix-linting-for-unbound-this branch July 19, 2022 07:55
hankucz pushed a commit to hankucz/faker that referenced this pull request Jul 19, 2022
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 p: 1-normal Nothing urgent
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants