📎 Implement useConsistentStringFunctions - unicorn prefer-string-trim-start-end prefer-string-slice

[minht11]

Description

Implement unicorn/prefer-string-trim-start-end and unicorn/prefer-string-slice

The rules themeselves are straightforward, and naming useConsistentStringFunctions fits them, question I have should this rule include other string related operation rules such as prefer-string-replace-all and prefer-string-starts-ends-with if yes is the current name appropriate?

Will work on this rule.

[chansuke]

Can I pick this issue?

[minht11]

@chansuke I have started implementing it, but haven't worked on it for a while so sure why not. One insight I can provide, that substring and slice are not identical unicorn/prefer-string-slice provides a lot of complex fixes which require type inference or static analysis which biome does not support yet. For Biome lets provide simple fixes and in other cases diagnostic error is good enough.

[Conaclos]

ne insight I can provide, that substring and slice are not identical

Because of this we should certainly create a dedicate rule for unicorn/prefer-string-slice?

[minht11]

If we are only including unicorn/prefer-string-trim-start-end and unicorn/prefer-string-slice under useConsistentStringFunctions I think it's fine to keep them together, documentation wise it wouldn't be that hard to explain, link to MDN section about differences slice/substring would be enough in my opinion. And of course note about it in the rule diagnostic.

From implementation standpoint detection is not hard, fixes requiring type inferences are, but we don't need to implement them right now (if at all?), so it's fine.

The only case I see that maybe some users would like to disable substring part, I feel that is unlikely, but in that case we could provide an option.

Because of that I lean towards keeping them together.

[Conaclos]

Because of that I lean towards keeping them together.

Looks fair to me.

[minht11]

Based on discussion on #3039 (comment) this rule should be split into two separate ones. To allow easier implementation, documentation and being more consistent with other rules. Sorry for the trouble @chansuke

[chansuke]

@minht11
No problem.
Thank for your information.

[chansuke]

Should it be:

• useStringTrimStartEndMethod
• useStringSliceMethod

? Thanks.

@minht11 @ematipico @Conaclos

[Conaclos]

Should it be:

useStringTrimStartEndMethod
useStringSliceMethod

The unicorn rules don't retrieve type information. So, these rules apply to all methods named trimLeft, trimRight, substring, and substr. In this respect, we could call these rules:

• noTrimLeftRight
• noSubstr

Note that I use the negation form here because I think it is more appropriate and consistent with our existing rules.

In the future we might decide to introduce new rules that make use of type information. These rules could be called noStringTrimLeftRight and noStringSubstr.

I didn't add the Methods suffix because we have similar rules without this suffix, notably useFlatMap, noFlatMapIdentity, and noForEach. I am not opposed to add the suffix. My only concern is consistency. Any opinion?

[chansuke]

My intention was to provide users with clear and detailed information, even if it is redundant. However, if the following plan is highly likely to be implemented, I think it would be acceptable to include String.

In the future we might decide to introduce new rules that make use of type information.

But I also agree with emphasizing consistency and noTrimLeftRight and noSubstr is a good enough for user.

[minht11]

That is very interesting point noStringTrimLeftRight versus noTrimLeftRight, because yeah currently noTrimLeftRight will match on any value which calls trimLeft not just strings.

useFlatMap would not be good example because it doesn't only match arrays but also any iterator

Though while unicorn does use static analysis sometimes for fixes and parameters, it does not use it to find method itself and they still call it prefer-string

When we have type inference in Biome, but are in pure javascript environment where nothing is typed is Biome gonna stop finding issues for this rule? We can't know if value is a string, because everything is any, so in that regard nothing is gonna change, for that kind of project type inference does not help. I see type inference more like false positive prevention, in cases where we know categorically that value is never a string. That would not be breaking change, but a fix.

So my vote would be for to add word string in the rule name, because adding type inference would not be breaking change in my eyes.

Edit:

I started to look into how I would name array apis:
prefer-array-find - any iterator for find only array for findLast
prefer-array-flat-map - any iterator
prefer-array-index-of - exists only on arrays
prefer-array-some - any iterator

So technically only prefer-array-index-of would include word array useArrayIndexOf, others would be without it. Likely semantics are bit too difficult to pick in every case and easier to just be consistent and not include literal in it. So I changed my mind to prefer noTrimLeftRight.
I still stand by my point about types and not introducing the new rule.

[Conaclos]

Anyway, I think it is a small thing. Choose the name you prefer and I think we are good for that.

[Conaclos]

Taking a look at the implementation, I wonder if we should also handle cases like:

- const f = foo.substr;
+ const f = foo.slice;