Comments on some rules

[gasche]

From the reddit thread on this project. Cool project, thanks!

| [%expr String.sub [%e? s] 0 [%e? n] ] -> Some "Use Str.first_chars"
| [%expr String.sub [%e? s] [%e? n] (String.length [%e? s'] - [%e? n']) ]
  when expr_eq s s' && expr_eq n n'
  -> Some "Use Str.string_after"
| [%expr String.sub [%e? s] (String.length [%e? s'] - [%e? n]) [%e? n'] ]
  when expr_eq s s' && expr_eq n n'
  -> Some "Use Str.last_chars"

I would not replace uses of the String library by the Str library, which in particular is not thread-safe -- it is unrelated to the use of those specific functions, but I still avoid it for this reason.

| [%expr [ [%e? _] ] @ [%e? _]] -> Some "Use ::"

I think that this rule should be refined. For example I sometimes write front @ [ mid ] @ back, and I would not write front @ mid :: back instead.

| [%expr List.concat (List.map [%e? _] [%e? _])] ->
    Some "Useless concat_map"

I don't understand this rule.

[emillon]

Hi!

Glad you like this tool. There are still some rough edges, in particular it is very opinionated, not configurable, a bit prescriptive in the tone, and tied to our codebase, but I expect this to change so that it can fit more and more users.

I would not replace uses of the String library by the Str library, which in particular is not thread-safe -- it is unrelated to the use of those specific functions, but I still avoid it for this reason.

These specific functions seem to be unrelated to the rest of the module, which is why it seems innocuous to use them.

If users avoid the Str module, I'd argue that these functions could be moved in the String module instead. This can be done safely with a deprecation cycle, but I am not too familiar with the current stdlib policy and understand that it's always a sensitive subject :)

For example I sometimes write front @ [ mid ] @ back, and I would not write front @ mid :: back instead

I agree that it leads to strange results. This rule in particular is the same as what hlint outputs by default. Once the rules are configurable, I agree that this one should be opt-in only.

Some "Useless concat_map"

This is tied to a function that we use in our codebase and does concat + map in a tail-recursive way. It will be removed in the opam release. The "Use Option.default" rule is similar - it's related to a Option.default : 'a -> 'a t -> 'a we use here.

Thanks again for your comments - they will hopefully be adressed soon.

[gasche]

Ok, so you meant Use concat_map rather than Useless concat_map. I was confused by the Useless part. Thanks for the clarifications!

[emillon]

I opened bugs for the different required features and fixed the typo. Thanks!