infra(unicorn): prefer-string-slice

ST-DDT commented 1 month ago

Ref: #2439

2439

Enables the unicorn/prefer-string-slice lint rule.

netlify[bot] commented 1 month ago

Deploy Preview for fakerjs ready!

Name	Link
Latest commit	e888b877af5f2c1fbe07023290ccb81fa43daca0
Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/661cef4976e84900086d8d34
Deploy Preview	https://deploy-preview-2814.fakerjs.dev
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.95%. Comparing base (a9929e2) to head (e888b87).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## next #2814 +/- ## ======================================== Coverage 99.95% 99.95% ======================================== Files 2966 2966 Lines 212579 212581 +2 Branches 946 602 -344 ======================================== + Hits 212484 212495 +11 + Misses 95 86 -9 ``` | [Files](https://app.codecov.io/gh/faker-js/faker/pull/2814?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js) | Coverage Δ | | |---|---|---| | [src/modules/finance/index.ts](https://app.codecov.io/gh/faker-js/faker/pull/2814?src=pr&el=tree&filepath=src%2Fmodules%2Ffinance%2Findex.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js#diff-c3JjL21vZHVsZXMvZmluYW5jZS9pbmRleC50cw==) | `100.00% <100.00%> (ø)` | | | [src/modules/helpers/eval.ts](https://app.codecov.io/gh/faker-js/faker/pull/2814?src=pr&el=tree&filepath=src%2Fmodules%2Fhelpers%2Feval.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js#diff-c3JjL21vZHVsZXMvaGVscGVycy9ldmFsLnRz) | `96.56% <100.00%> (ø)` | | | [src/modules/helpers/index.ts](https://app.codecov.io/gh/faker-js/faker/pull/2814?src=pr&el=tree&filepath=src%2Fmodules%2Fhelpers%2Findex.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js#diff-c3JjL21vZHVsZXMvaGVscGVycy9pbmRleC50cw==) | `98.38% <100.00%> (+<0.01%)` | :arrow_up: | | [src/modules/internet/index.ts](https://app.codecov.io/gh/faker-js/faker/pull/2814?src=pr&el=tree&filepath=src%2Fmodules%2Finternet%2Findex.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js#diff-c3JjL21vZHVsZXMvaW50ZXJuZXQvaW5kZXgudHM=) | `100.00% <100.00%> (ø)` | | | [src/modules/lorem/index.ts](https://app.codecov.io/gh/faker-js/faker/pull/2814?src=pr&el=tree&filepath=src%2Fmodules%2Florem%2Findex.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js#diff-c3JjL21vZHVsZXMvbG9yZW0vaW5kZXgudHM=) | `100.00% <100.00%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/faker-js/faker/pull/2814/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=faker-js)

ST-DDT commented 1 month ago

Please pay extra attention when reviewing the files changed by f48dfe4 because I had to convert them by hand and I don't know why.

xDivisionByZerox commented 1 month ago

From the docs:

String#substr() and String#substring() are the two lesser known legacy ways to slice a string.

Have to disagree on the String#substring part. Without having prove of it, I would say that most JS developers would know String#substring while being unaware of String#slice 🤔

ST-DDT commented 1 month ago

I can also disable the rule permanently.

xDivisionByZerox commented 4 weeks ago

While I have some doubts about the previously explained statement, this rule would at least unify the way string slicing would be performed in the project. This might be a DX improvement (for faker developers) in itself because one has not wondered why X is used in one place while Y is used in another.

Shinigami92 commented 4 weeks ago

My personal rules:

never use substr 🙅 its the most deprecated one
use substring in specific cases (as far as I know it has some specific behavior, but I would need always to look these up)
mostly use slice
- pros: it is well known, acts like for arrays so some can imagine exactly how it works by thinking that each char is just an array element, and some can use shortways like -1 to address elements from the end
- cons: in minor cases it is confusing if there is currently a string or an array, however in todays world using TypeScript, this is not a problem anymore as you have highlighting and code tooltips

Examples:

- filePath.substring(pathLocales.length + 1, filePath.length - 3)
+ filePath.slice(pathLocales.length + 1, -3)

this is a really good change, and I like it

- fileContent.substring(0, compareIndex)
+ fileContent.slice(0, Math.max(0, compareIndex))

this is not so good, because it complexifies the code by using Math.max and some need to more think about what's going on -> Looks like the thing is that substring(_, negativeValue) results always in empty string.

TBH I personally like use substring for such cases then... 👀 Does the lint-rule has some options to configure it?

matthewmayer commented 4 weeks ago

This only really makes sense if we manually go through cases like this and remove the ugly Math.max in places where we know compareIndex must be positive.

fileContent.slice(0, Math.max(0, compareIndex))

faker-js / faker