Allow disabling of regexp character sorting

[voxpelli]

Fixes #453

Improved by DmitrySoshnikov/regexp-tree#200

Does this look like a good addition?

[sindresorhus]

The description here needs to be clearer about what exactly will be sorted. "characters" is too ambiguous.

[voxpelli]

@sindresorhus There are two options as I see it:

1. Get into the specifics of regexp-tree and be up front about the disabling of charClassClassrangesMerge, which the sorting is a needed side-effect of. This is probably too implementation specific and doesn't convey the purpose clearly.
2. Clearly convey the intended effect by naming it sortCharacters/sortCharacterClasses (which the description of maybe could be spelled out in a somewhat more specific manner, but I have a hard time pinpointing that, maybe due to me not being the best at Regexp terminology).

I lean towards the second and I get the impression that you do as well?

[sindresorhus]

Sorry, missed this. Yes, 2. Could you also add an example? Not everyone knows what character classes are.

[fisker]

Does charClassClassrangesMerge can actually disable sorting?
If it does, I suggestion use charClassClassrangesMerge directly.

Also we probably want use object like this

eslint-plugin-unicorn/rules/prevent-abbreviations.js

Line 16 in cfd3d49

const defaultReplacements = {

so options will like

{
  charClassClassrangesMerge: false,
  charSurrogatePairToSingleUnicode: true,
}

[fisker]

@voxpelli great work, indeed this prevent sorting

[fisker]

@voxpelli would you mind if I refactor the option logic? we don't need that blacklist logic.

[voxpelli]

@fisker Because of reasons outlined in #468 (comment) I think the logic around regexp-tree should be kept as an internal implementation of this rule and not be exposed to the outside world.

That way this rule can more easily be adapted to future changes in regexp-tree and/or to future alternative modules.

Therefore I think that all we should do for now is to convey to regexp-tree that we want this very rule to sometimes be disabled, as made possible through DmitrySoshnikov/regexp-tree#200

Reason why I've implemented the blacklist feature myself here is to make this feature be easily upgraded once that PR has been merged and to have the intention in this code to be clear,

I do not like the idea of:

{
  charClassClassrangesMerge: false,
  charSurrogatePairToSingleUnicode: true,
}

That exposes way too much about the internal implementation and pushes too many decisions onto the user rather than abstract them in a way that makes sense from the purpose of this rule (which has a different target audience than the one of regexp-tree).

[fisker]

@voxpelli Your comment make sense, but I still think let users have all the options is good.

Also this charClassClassrangesMerge not only prevent sorting, also disable ranges from merging as you can see

/[GgHhIiå.Z:a-f"0-8%A*ä]/

/["%*.0-8:AG-IZa-iäå]/

the a-fghi is merged to a-i, we disable charClassClassrangesMerge also disable merging, people may ask why

[voxpelli]

@fisker The fact that it disables is a reason why I want to expose the disabling of sorting, as that enables this rule to evolve to separate those additional fixes from the actual sorting, which a raw option of charClassClassrangesMerge totally dodges and instead downstreams to regexp-tree, where it may not belong as clearly.

Such a shortcoming can be mentioned in the documentation of a sortCharacterClasses option and a future version of this module can then fix it, when regexp-tree has evolved to such a degree that we can actually fix it.

[fisker]

I have a regex /Error:?\s+([^\r\n]+)/i, yesterday when I update unicorn, I got it fixed to /error:?\s+([^\n\r]+)/i, in fact, my test string is Error:...., I put i there just in case I may change the message in feature for safty, if I can set charCaseInsensitiveLowerCaseTransform, I may already change my default, because I prefer not touching it.

I guess not only me, people may also need this. If it happends, we'll need to add another option again. Why not make it easier

[voxpelli]

I think we disagree on what makes it easier 🙂

I think exposing of internal implementation details makes the API more complicated while you consider it to make it easier.

I guess we can agree to disagree and have someone else, like @sindresorhus, chime in with his views?

[fisker]

//cc @futpib @MrHen may also interested

[sindresorhus]

I agree with @voxpelli, we shouldn't expose the internals of regexp-tree.

[sindresorhus]

I put i there just in case I may change the message in feature for safty

I would argue this is actually an argument for not exposing all the options. The rule is actually improving your code and you're trying to fight it. By having the i in there, even though it's not used, you're actually making your regex harder to read. I think your "just in case" need is better solved by an explanatory code comment.

I guess not only me, people may also need this. If it happends, we'll need to add another option again.

If there's a valid use-case, we can add a well-documented option for it. Exposing everything just leads to users having to waste too much time reading docs and configuring, and we would have to pin the dependency as every patch release of regexp-tree is a potential breaking change for us.

[sindresorhus]

@voxpelli The update is out: https://github.com/DmitrySoshnikov/regexp-tree/commits/master Can you update to use the blacklist?

[fisker]

@sindresorhus @voxpelli I did the request changes

[voxpelli]

Thanks @fisker! I was out of office for a few days, so unfortunately didn't have time to fix it :)

[voxpelli]

@sindresorhus Is this ready to go you think?

[sindresorhus]

@voxpelli Did you see #468 (comment)?

[sindresorhus]

@voxpelli Bump :)

[voxpelli]

Done @sindresorhus :) Sorry for me taking so long, I appreciate your bumps to remind me 👍

[voxpelli]

Also rebased it 👍

[sindresorhus]

Thank you 👍

[JounQin]

@sindresorhus Should its default be false due to #472, #477, #499?

[sindresorhus]

I'm hoping that the author of regexp will fix those extra issues soon. If not, we could do this, yes.