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: add eslint unicorn #2417

Closed
wants to merge 1 commit into from
Closed

chore: add eslint unicorn #2417

wants to merge 1 commit into from

Conversation

Shinigami92
Copy link
Member

Based on #2284 (comment)

This PR will make @fisker really happy ❤️

@Shinigami92 Shinigami92 added c: dependencies Pull requests that adds/updates a dependency c: infra Changes to our infrastructure or project setup labels Sep 23, 2023
@Shinigami92 Shinigami92 added this to the vAnytime milestone Sep 23, 2023
@Shinigami92 Shinigami92 self-assigned this Sep 23, 2023
@Shinigami92 Shinigami92 changed the title infra: add eslint unicorn chore: add eslint unicorn Sep 23, 2023
@Shinigami92 Shinigami92 added c: chore PR that doesn't affect the runtime behavior and removed c: infra Changes to our infrastructure or project setup labels Sep 23, 2023
@Shinigami92 Shinigami92 force-pushed the add-eslint-unicorn branch 4 times, most recently from be40894 to 1c2d022 Compare September 23, 2023 17:56
],
parser: '@typescript-eslint/parser',
parserOptions: {
project: ['./tsconfig.json'],
sourceType: 'module',
warnOnUnsupportedTypeScriptVersion: false,
},
plugins: ['@typescript-eslint', 'prettier', 'deprecation'],
plugins: ['@typescript-eslint', 'prettier', 'deprecation', 'unicorn'],
Copy link
Member

Choose a reason for hiding this comment

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

This is optional/already covered by the extends config.

'error',
{
allowList: {
args: true,
Copy link
Member

Choose a reason for hiding this comment

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

Why args but not arg?

test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
@@ -461,7 +461,7 @@ describe('helpers', () => {
const input = Array.from({ length: 2 }, (_, i) => i);
const occurrences = Array.from({ length: 2 }, () => 0);

for (let i = 0; i < 1000; i++) {
for (let index = 0; index < 1000; index++) {
Copy link
Member

Choose a reason for hiding this comment

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

This isnt the index it is a counter.

test/modules/helpers.spec.ts Outdated Show resolved Hide resolved
allowList: {
args: true,
fn: true,
obj: true,
Copy link
Member

Choose a reason for hiding this comment

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

Why obj, but not str/num?

@@ -44,8 +44,8 @@ describe('color', () => {

describe(`cssSupportedFunction()`, () => {
it('should return random css supported color function from css functions array', () => {
const func = faker.color.cssSupportedFunction();
expect(Object.values(CssFunction)).toContain(func);
const supportedFunction = faker.color.cssSupportedFunction();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe name it actual same as in our other tests?

@@ -358,11 +358,11 @@ describe('date', () => {
expect(dates.length).lessThanOrEqual(5);

expect(dates[0]).greaterThan(from);
for (let i = 1; i < dates.length; i++) {
expect(dates[i]).greaterThan(dates[i - 1]);
for (let index = 1; index < dates.length; index++) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnessarily verbose.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 23, 2023

Note to self. Check all the regexes again.

@Shinigami92
Copy link
Member Author

@ST-DDT I highly suggest you start reviewing this when I'm actually done with it
There are more then 800 errors I need to manually go through
I'm already sitting on this more than an hour and I make a break now, playing Overwatch

@codecov
Copy link

codecov bot commented Sep 23, 2023

Codecov Report

Merging #2417 (456698f) into next (74eeccc) will decrease coverage by 0.01%.
The diff coverage is 97.34%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #2417      +/-   ##
==========================================
- Coverage   99.60%   99.60%   -0.01%     
==========================================
  Files        2802     2802              
  Lines      252486   252544      +58     
  Branches     1103     1101       -2     
==========================================
+ Hits       251499   251553      +54     
- Misses        960      964       +4     
  Partials       27       27              
Files Changed Coverage Δ
src/definitions/science.ts 0.00% <0.00%> (ø)
src/definitions/system.ts 0.00% <0.00%> (ø)
src/locales/base/system/directory_paths.ts 100.00% <ø> (ø)
src/locales/base/system/mime_types.ts 100.00% <ø> (ø)
src/locales/en/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/eo/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/nb_NO/science/chemical_element.ts 100.00% <ø> (ø)
src/locales/pl/science/chemical_element.ts 100.00% <ø> (ø)
src/modules/internet/user-agent.ts 91.17% <87.50%> (-0.25%) ⬇️
src/modules/person/index.ts 98.01% <90.00%> (+<0.01%) ⬆️
... and 38 more

const occurrences = Array.from({ length: 10 }, () => 0);

for (let i = 0; i < 1000; i++) {
for (let index = 0; index < 1000; index++) {
Copy link
Member

Choose a reason for hiding this comment

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

This isnt an index.

@@ -485,7 +485,7 @@ describe('image', () => {

expect(imageUrl).toBeTypeOf('string');
expect(imageUrl).toMatch(
/^https\:\/\/via\.placeholder\.com\/\d+x\d+\/[0-9a-fA-F]{6}\/[0-9a-fA-F]{6}\.[a-z]{3,4}\?text=.+$/
/^ht{2}ps:\/{2}via\.placeholder\.com\/\d+x\d+(?:\/[\dA-Fa-f]{6}){2}\.[a-z]{3,4}\?text=.+$/
Copy link
Member

Choose a reason for hiding this comment

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

No. ht{2}ps:\/{2} 🙅

@@ -297,7 +297,7 @@ describe('internet', () => {
const [prefix, suffix] = email.split('@');

expect(prefix).toMatch(
/^Mike((\d{1,2})|([.!#$%&'*+-/=?^_`{|}~]Smith\d{1,2})|([.!#$%&'*+-/=?^_`{|}~]Smith))/
/^Mike((\d{1,2})|([!#$%&'*+-/=?^_`{|}~]Smith\d{1,2})|([!#$%&'*+-/=?^_`{|}~]Smith))/
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: Where did the dot go?

test/modules/lorem.spec.ts Show resolved Hide resolved
@@ -142,7 +142,7 @@ class TestGenerator<
repetitions: number = 1
): void {
this.setup();
for (let i = 0; i < repetitions; i++) {
for (let index = 0; index < repetitions; index++) {
Copy link
Member

Choose a reason for hiding this comment

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

This isnt an index.

@ST-DDT
Copy link
Member

ST-DDT commented Sep 23, 2023

@ST-DDT I highly suggest you start reviewing this when I'm actually done with it There are more then 800 errors I need to manually go through I'm already sitting on this more than an hour and I make a break now, playing Overwatch

I'm not sure I want to review 800 changes in one PR.

Correction: The current number of addressed (types of) issues already exceeds what I'm willing to review in a single PR.

@ST-DDT ST-DDT added s: needs decision Needs team/maintainer decision s: on hold Blocked by something or frozen to avoid conflicts labels Sep 23, 2023
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

This PR is not reviewable due to size/different kind of changes.

@Shinigami92 Shinigami92 force-pushed the add-eslint-unicorn branch 4 times, most recently from 57d0a9e to b27a254 Compare September 23, 2023 20:37
@Shinigami92
Copy link
Member Author

As @ST-DDT already pointed out, I need to split this into several PRs, to reduce the amount of changed files in the same PR.

This PR now reflects the approximate change when everything is merged individually.

While working on this PR, I at least learned some stuff I can reuse to work on the individual PRs.
And maybe this PR gets so much reduces in diff count, that it can then be used to check the rest that is left over.

@Shinigami92 Shinigami92 added do NOT merge yet Do not merge this PR into the target branch yet and removed s: needs decision Needs team/maintainer decision s: on hold Blocked by something or frozen to avoid conflicts labels Sep 23, 2023
@Shinigami92
Copy link
Member Author

I switched from on hold to do not merge, because on hold is for that the work is not going forward, which is effectively wrong in this case

@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Sep 25, 2023
@ST-DDT
Copy link
Member

ST-DDT commented Oct 6, 2023

This has been superseded by #2418 and individual PRs per rule.

Since Shinigami92 is no longer actively working on this. I will close this because of the merge conflicts.
The diff is visible here forever and the branch can be restored if needed as well.

@ST-DDT ST-DDT closed this Oct 6, 2023
@ST-DDT ST-DDT deleted the add-eslint-unicorn branch October 6, 2023 23:50
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 c: dependencies Pull requests that adds/updates a dependency do NOT merge yet Do not merge this PR into the target branch yet needs rebase There is a merge conflict
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants