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

Added regexp/no-obscure-range rule #122

Merged
merged 14 commits into from
Apr 16, 2021
Merged

Added regexp/no-obscure-range rule #122

merged 14 commits into from
Apr 16, 2021

Conversation

RunDevelopment
Copy link
Collaborator

This is the clean-regex/no-obscure-range rule.

I added it to recommended because it mainly finds bugs.

Since I added new util methods, I also made regexp/no-octal use one of them.

@RunDevelopment
Copy link
Collaborator Author

@ota-meshi Maybe we should add a package-lock.json after all?

The build failed not because of the files I added/changed. The build failed because some dependency just released a new version with some breaking change. Because of this, ALL PRs will now fail until we find a solution for this.

@ota-meshi
Copy link
Owner

The cause of the build break was a change in @types/eslint released 4 hours ago.
https://www.npmjs.com/package/@types/eslint
I added a lock file. #124

@ota-meshi
Copy link
Owner

I added it to recommended because it mainly finds bugs.

I want to do recommended: true in v1.0.0 because I want to treat configuration changes as breaking changes like ESLint.
I want to add a TODO comment to make it recommended: false.

@RunDevelopment
Copy link
Collaborator Author

I want to treat configuration changes as breaking changes like ESLint.

We are currently in version 0.x. This is the initial development phase according to semver. npm's caret operator will treat 0.x versions in a special way.

From my understanding, we are allowed to introduce breaking changes from minor to minor as long as the major is 0.

@ota-meshi
Copy link
Owner

We are currently in version 0.x. This is the initial development phase according to semver. npm's caret operator will treat 0.x versions in a special way.
From my understanding, we are allowed to introduce breaking changes from minor to minor as long as the major is 0.

It's exactly as you said. But I would like to release v1.0 once we have implemented the clean-regex rules.
I think it's in the near future, so I'd like to include the configuration changes in #49 and release it in v1.0.
This plugin has an increasing number of users (unexpectedly 😅), so I don't want to make many breaking changes even with v0.x.

@RunDevelopment
Copy link
Collaborator Author

But I would like to release v1.0 once we have implemented the clean-regex rules.

I would say that we should release one last 0.x release after implementing the clean-regex rules. Frankly, I would like to test out all rules working together in practice for one or two weeks. Issues have been piling up over at Prism, so I will write a lot of regular expressions soon.

v1.0 means that we now guarantee stability and I would like to thoroughly test this plugin before that.

I think it's in the near future

I agree. I expect that we will have moved over most of clean-regex' rules by the end of April.

This plugin has an increasing number of users (unexpectedly 😅), so I don't want to make many breaking changes even with v0.x.

We are allowed to make breaking changes but won't to make things easier for our users? I can accept that reasoning.

@RunDevelopment
Copy link
Collaborator Author

Changes:

  • New regexp/allowed-character-ranges setting and new allowed option for no-obscure-ranges.
    Those two and the target option from prefer-ranges all have the same semantics & schema.
  • If both the allowed/target option and the regexp/allowed-character-ranges setting are defined, then the allowed/target option will override the setting. (This makes the change backward compatible.)
  • Fixed a bug in prefer-ranges. This condition didn't verify 1) that both min and max are allowed ranges and 2) that min and max are both of the same range. This had the consequence that prefer-ranges was happy to transform [a-zA-ZZ-a] to [A-z].

To do:

  • Document the regexp/allowed-character-ranges setting and the allowed option. Both should get the same documentation as the target option currently has. This might best be done but creating a new page to document settings. The doc for allowed/target can then reference the settings page.
  • Rename the setting. Are there best practices for naming settings? I just used our plugin prefix (not required) and then "allowed-character-ranges". Not sure if that's a good name.
  • Rename allowed or target. Both options basically do the same but they have different names.

@ota-meshi Thoughts ?

@ota-meshi
Copy link
Owner

Thank you for a lot of work!

Rename the setting. Are there best practices for naming settings? I just used our plugin prefix (not required) and then "allowed-character-ranges". Not sure if that's a good name.

eslint-plugin-html seems to use a name like "settings": {"namespace/kebab-case": value}.
https://www.npmjs.com/package/eslint-plugin-html#settings
eslint-plugin-node and eslint-plugin-es seem to use a format like "settings": {"namespace ": { "camelCase": value }}.
https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-missing-import.md#shared-settings
https://eslint-plugin-es.mysticatea.dev/#the-aggressive-mode

I think it's better to use the same naming that mysticatea uses, so I think the setting should be "settings": { "regexp": { "allowedCharacterRanges": value } }.

Rename allowed or target. Both options basically do the same but they have different names.

I think it's okay if the options in both rules have different names.
The prefer-ranges rule sets "target" that prefer ranges.
The no-obscure-ranges rule sets the "allowed" ranges without escaping.

Fixed a bug in prefer-ranges

Thank you for finding and fixing bug!

@RunDevelopment RunDevelopment linked an issue Apr 16, 2021 that may be closed by this pull request
@RunDevelopment
Copy link
Collaborator Author

I think it's better to use the same naming that mysticatea uses, so I think the setting should be "settings": { "regexp": { "allowedCharacterRanges": value } }.

I like that.

Now, the only thing left is to add some doc.

@RunDevelopment
Copy link
Collaborator Author

I added some docs but I'm not sure if we want to keep it that way. I just added a new "Settings" page that explains what settings are and lists all settings.

In the future, we might add more settings and each setting should probably get its own page. Should I do this now or do we wait until we add a second setting?

@ota-meshi
Copy link
Owner

Thank you for changing this PR!
I checked this PR locally and I like the composition of this document!
I would like to reconfirm the others a bit more and merge this PR.

Copy link
Owner

@ota-meshi ota-meshi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

@ota-meshi ota-meshi merged commit fe6eced into master Apr 16, 2021
@ota-meshi ota-meshi deleted the no-obscure-range branch April 16, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: Forbid range Z-a
2 participants