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

[Fix #9558] Fix inconsistency when dealing with URIs that are wrapped in single quotes vs double quotes #9568

Merged
merged 1 commit into from Mar 10, 2021

Conversation

dvandersluis
Copy link
Member

@dvandersluis dvandersluis commented Mar 8, 2021

The LineLength cop uses LineLengthHelp#allowed_uri_position? to determine if the URI is in an allowed position. Previously it checked that the URL either ended the line, or ended one character before the end of the line:

def allowed_uri_position?(line, uri_range)
uri_range.begin < max_line_length &&
(uri_range.end == line_length(line) ||
uri_range.end == line_length(line) - 1)
end

It looks like this was added originally in #5361... also to handle disparity between single and double quotes. 🤔

As it turns out, ruby's default URI parser (which we use to match parts of strings as a URL), accepts ' as a valid character, but does not accept ". It also matches ). Therefore, the URL extracted from the string differs depending on the type of quote being used:

regexp = URI::DEFAULT_PARSER.make_regexp(['http', 'https'])
regexp.match(%q[http://foo.com/')])
=> #<MatchData "http://foo.com/')" 1:"http" 2:nil 3:nil 4:"foo.com" 5:nil 6:nil 7:"/')" 8:nil 9:nil>
regexp.match(%q[http://foo.com/")])
=> #<MatchData "http://foo.com/" 1:"http" 2:nil 3:nil 4:"foo.com" 5:nil 6:nil 7:"/" 8:nil 9:nil>

This was leading to the failure in the issue, because for the single quoted string, the "URL" ends on the last character (despite the ') not actually being a part of the URL!), but for the double quoted string, the parser actually ends at the "real" end of the URL, and then allowed_uri_position? fails.

This change updates the uri_range to also capture any trailing characters until the next whitespace, and then just checks that the uri_range ends at the end of the line. This fixes the issue while also continuing to register an offense for subsequent words.

Fixes #9558.


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.

…wrapped in single quotes vs double quotes.
Copy link
Collaborator

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

Nicely done!

@bbatsov bbatsov merged commit 93711ca into rubocop:master Mar 10, 2021
@dvandersluis dvandersluis deleted the issue/9558 branch March 10, 2021 14:00
koic added a commit to rubocop/rubocop-performance that referenced this pull request Mar 13, 2021
Follow rubocop/rubocop#9568.

This PR suppresses the following offense.

```console
% bundle exec rake
(snip)

Offenses:

rubocop-performance.gemspec:28:5: W: [Correctable]
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Layout/LineLength.
# rubocop:disable Layout/LineLength
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

104 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
renawatson68 added a commit to renawatson68/performance-develop-rubyonrails that referenced this pull request Sep 23, 2022
Follow rubocop/rubocop#9568.

This PR suppresses the following offense.

```console
% bundle exec rake
(snip)

Offenses:

rubocop-performance.gemspec:28:5: W: [Correctable]
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Layout/LineLength.
# rubocop:disable Layout/LineLength
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

104 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
richardstewart0213 added a commit to richardstewart0213/performance-build-Performance-optimization-analysis- that referenced this pull request Nov 4, 2022
Follow rubocop/rubocop#9568.

This PR suppresses the following offense.

```console
% bundle exec rake
(snip)

Offenses:

rubocop-performance.gemspec:28:5: W: [Correctable]
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Layout/LineLength.
# rubocop:disable Layout/LineLength
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

104 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
MarttiCheng added a commit to MarttiCheng/Rubocop-Performance that referenced this pull request Sep 28, 2023
Follow rubocop/rubocop#9568.

This PR suppresses the following offense.

```console
% bundle exec rake
(snip)

Offenses:

rubocop-performance.gemspec:28:5: W: [Correctable]
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Layout/LineLength.
# rubocop:disable Layout/LineLength
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

104 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
SerhiiMisiura added a commit to SerhiiMisiura/Rubocop-Performance that referenced this pull request Oct 5, 2023
Follow rubocop/rubocop#9568.

This PR suppresses the following offense.

```console
% bundle exec rake
(snip)

Offenses:

rubocop-performance.gemspec:28:5: W: [Correctable]
Lint/RedundantCopDisableDirective: Unnecessary disabling of
Layout/LineLength.
# rubocop:disable Layout/LineLength
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

104 files inspected, 1 offense detected, 1 offense auto-correctable
RuboCop failed!
```
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.

Inconsistent LineLength behavior with URL-like strings
2 participants