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(user-agent): fix linter warnings #1174

Merged
merged 11 commits into from
Jul 30, 2022

Conversation

xDivisionByZerox
Copy link
Member

@xDivisionByZerox xDivisionByZerox commented Jul 20, 2022

This PR fixes all remaining linter warnings in user-agent.ts.

To achieve this I changed the implementation of the function in a non-breaking way. I mainly simplified the rnd function since it had a lot of unnecessary branches. I also converted all internal functions to constants so they don't require any JSDoc.

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

BTW, this PR brings the total number of linter warnings in the project down to ZERO 🚀

@xDivisionByZerox xDivisionByZerox requested a review from a team July 20, 2022 17:14
@codecov
Copy link

codecov bot commented Jul 20, 2022

Codecov Report

Merging #1174 (a399b84) into main (02ead05) will decrease coverage by 0.00%.
The diff coverage is 77.68%.

@@            Coverage Diff             @@
##             main    #1174      +/-   ##
==========================================
- Coverage   99.63%   99.62%   -0.01%     
==========================================
  Files        2152     2152              
  Lines      236201   236234      +33     
  Branches      980      974       -6     
==========================================
+ Hits       235336   235347      +11     
- Misses        844      866      +22     
  Partials       21       21              
Impacted Files Coverage Δ
src/modules/internet/user-agent.ts 81.74% <77.68%> (-4.64%) ⬇️

src/modules/internet/user-agent.ts Outdated Show resolved Hide resolved
src/modules/internet/user-agent.ts Outdated Show resolved Hide resolved
src/modules/internet/user-agent.ts Outdated Show resolved Hide resolved
@xDivisionByZerox xDivisionByZerox force-pushed the chore/linter/fix-user-agent-warnings branch from d2f0baa to 55ec3a2 Compare July 25, 2022 21:05
@xDivisionByZerox xDivisionByZerox requested review from a team July 25, 2022 21:05
@xDivisionByZerox
Copy link
Member Author

I can't reproduce the error described in the failing pipeline. Anyone may help?

@ST-DDT
Copy link
Member

ST-DDT commented Jul 26, 2022

I can't reproduce the error described in the failing pipeline. Anyone may help?

This is a relatively new error that only happens randomly.
I only managed to reproduce it once or twice locally, but I don't know what's causing this.
We could increase the timeout for that test, but I'm not sure whether that's the actual issue or whether the imports in there examples get resolved strangely and thus appear to hang for a "short" moment. (Thus it always happens in a different test with many imports).
This might or might not be related to the long test start times that a few users have reported which we then recommended to use the locale specific faker instance.

I ran the test again and now it is working.

ST-DDT
ST-DDT previously approved these changes Jul 26, 2022
@xDivisionByZerox
Copy link
Member Author

This might or might not be related to the long test start times that a few users have reported which we then recommended to use the locale specific faker instance.

I see. Can this be due to all modules importing Faker from the src/index.ts file and creating a circular reference in the process (ModuleX => importing Faker from index.ts => index.ts import ModuleX)? This is just a guess, I have no idea how dependency resolution works in JS.

@xDivisionByZerox xDivisionByZerox force-pushed the chore/linter/fix-user-agent-warnings branch from 55ec3a2 to f6699e4 Compare July 28, 2022 20:59
@xDivisionByZerox xDivisionByZerox added the m: internet Something is referring to the internet module label Jul 28, 2022
@ST-DDT ST-DDT requested a review from a team July 29, 2022 09:04
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Jul 29, 2022
@xDivisionByZerox
Copy link
Member Author

@ST-DDT the accepted label is described for accepted features or confirmed bugs. Not for chores 🤔

@ST-DDT ST-DDT requested a review from a team July 29, 2022 20:54
@ST-DDT ST-DDT merged commit 09df73a into main Jul 30, 2022
@ST-DDT ST-DDT deleted the chore/linter/fix-user-agent-warnings branch July 30, 2022 09:20
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 m: internet Something is referring to the internet module p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants