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

Fix all eslint warnings #155

Open
dawsbot opened this issue Aug 6, 2022 · 6 comments
Open

Fix all eslint warnings #155

dawsbot opened this issue Aug 6, 2022 · 6 comments
Labels
good first issue Good for newcomers

Comments

@dawsbot
Copy link
Owner

dawsbot commented Aug 6, 2022

Can be found via npm run lint

Screen Shot 2022-08-05 at 8 32 14 PM

@dawsbot dawsbot added the good first issue Good for newcomers label Aug 6, 2022
@erum-hassan
Copy link

I want to take it.

@dawsbot
Copy link
Owner Author

dawsbot commented Aug 8, 2022

Go for it @erum-hassan ! 🙏

@dawsbot
Copy link
Owner Author

dawsbot commented Sep 12, 2022

Hey @erum-hassan , any updates on this?

@jtfirek
Copy link
Collaborator

jtfirek commented Apr 5, 2023

Hey @dawsbot I was going to knock this one out for you. It's seems like you got must of the lint issues. There are only these remaining that all come from this file:
https://github.com/dawsbot/essential-eth/blob/ae50d17a1040c46c7149e9ff585dce8c9e65dfc9/src/providers/test/json-rpc-provider/get-fee-data.test.ts

/Users/jacobfirek/Documents/Projects/essential-eth/essential-eth/src/providers/test/json-rpc-provider/get-fee-data.test.ts
  16:7  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  19:5  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  21:7  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  24:5  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  26:7  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  29:5  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment
  31:7  warning  Do not use "@ts-ignore" because it alters compilation errors  @typescript-eslint/ban-ts-comment

I have thought of 3 solutions:

  • Add an exception to the lint rules for @ts-ignore.
  • set a default value incase of null.
  • Use the ! non-null assertion which would suppress the original warning, but throw a new warning warning Forbidden non-null assertion

Which would you like me to do or would you like to see this fixed in another way?

@dawsbot
Copy link
Owner Author

dawsbot commented Apr 6, 2023

Thanks for the detailed thoughts here @jtfirek - unfortunately I think the design of these tests is flawed at a big level because they are so dependent upon both ethers and having network access.

I'm hoping to provide a proper refactor to tests like this using mocks so that it's no-longer network-dependent and flaky. Do you have experience with mocking?

@jtfirek
Copy link
Collaborator

jtfirek commented Apr 6, 2023

I do not, but I'll learn it! I will put #201 on my todo list @dawsbot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants