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

Support free-spacing mode Regexp in Naming/InclusiveLanguage #11832

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sambostock
Copy link
Contributor

WIP – See #11831.

This edits the way the Naming/InclusiveLanguage cop combines its config into Regexp to match terms. Specifically, because we would convert given Regexp to Strings via Regexp#source, and then simply combine patterns/strings using

Regexp.new(strings.join('|'), Regexp::IGNORECASE)

we would lose any mode/options information about the Regexp, which in the case of "free-spacing"/"extended" mode, would mean the Regexp could be corrupted by additional spacing being considered part of the pattern.

This changes the approach to normalize patterns as Regexps instead of Strings, and then simply combine them using Regexp.union. To preserve the existing behaviour of always making the patterns case insensitive, we extract the source and options from any given Regexp, and construct a new one where we force case insensitivity using

Regexp.new(regexp.source, regexp.options | Regexp::IGNORECASE)

This way, we preserve all other flags (/.../x, but also others like /.../m).

Note that it was previously possible to work around this by applying options at the subexpression level, which remains possible (e.g. to force case sensitivity).

TODO

  • Validate approach
  • Rename identifiers accordingly
  • Add more specs for edge cases and covering existing behaviour, to ensure it is preserved.
  • Better document use of Regexp in cop documentation (unclear how AllowedRegexp works; maybe separate PR?)

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
  • Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

regex.is_a?(Regexp) ? regex.source : regex
def ensure_case_insensitive_regexp(object)
case object
when Regexp then Regexp.new(object.source, object.options | Regexp::IGNORECASE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could optimize to skip creating duplicates

Suggested change
when Regexp then Regexp.new(object.source, object.options | Regexp::IGNORECASE)
when Regexp
return object if object.options == Regexp::IGNORECASE
Regexp.new(object.source, object.options | Regexp::IGNORECASE)

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.

None yet

1 participant