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.

Sign up for GitHub

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

Jump to bottom

Fix rubocop redundant freeze warnings #5468

Merged
merged 1 commit into from
Oct 25, 2022
Merged

Fix rubocop redundant freeze warnings #5468

merged 1 commit into from
Oct 25, 2022

Conversation

jeffwidman
Copy link
Member

@jeffwidman jeffwidman commented Aug 3, 2022 •
edited

Fix Rubocop freeze warnings.

Rubocop is warning about things that are double frozen, ie .freeze is redundant.

These warnings started showing up as part of the Ruby 3.1 upgrade, and
were excluded from that PR in order to minimize noise.

@jeffwidman jeffwidman mentioned this pull request Aug 3, 2022
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 3, 2022
View reviewed changes
composer/lib/dependabot/composer/helpers.rb
@@ -6,12 +6,12 @@
module Composer
module Helpers
# From composers json-schema: https://getcomposer.org/schema.json
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]+)*$}.freeze
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]+)*$}

Check failure

Code scanning / CodeQL

Inefficient regular expression

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '0'.
composer/lib/dependabot/composer/helpers.rb
@@ -6,12 +6,12 @@
module Composer
module Helpers
# From composers json-schema: https://getcomposer.org/schema.json
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]+)*$}.freeze
COMPOSER_V2_NAME_REGEX = %r{^[a-z0-9]([_.-]?[a-z0-9]+)*/[a-z0-9](([_.]?|-{0,2})[a-z0-9]+)*$}

Check failure

Code scanning / CodeQL

Inefficient regular expression

This part of the regular expression may cause exponential backtracking on strings containing many repetitions of '0'.
@jeffwidman jeffwidman force-pushed the fix-rubocop-freeze-warnings branch 2 times, most recently from 7a97e8f to 655de48 Compare August 7, 2022 22:29
@jeffwidman jeffwidman force-pushed the fix-rubocop-freeze-warnings branch 2 times, most recently from 96eccbf to a534e2a Compare September 13, 2022 01:18
@jeffwidman
Copy link
Member Author

I had to do a painful rebase on this one, and easily could have made a mistake. So once #5447 is merged and this is rebased on top of that, do a careful inspection of the changes to ensure I didn't screw it up.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 13, 2022
View reviewed changes
python/lib/dependabot/python/requirement_parser.rb
COMPARISON = /===|==|>=|<=|<|>|~=|!=/
VERSION = /([1-9][0-9]*!)?[0-9]+[a-zA-Z0-9\-_.*]*(\+[0-9a-zA-Z]+(\.[0-9a-zA-Z]+)*)?/

REQUIREMENT = /(?<comparison>#{COMPARISON})\s*\\?\s*(?<version>#{VERSION})/

Check warning

Code scanning / CodeQL

Overly permissive regular expression range

Suspicious character range that is equivalent to \[)*+,\-.\/0-9\].
jeffwidman
jeffwidman commented Oct 11, 2022
View reviewed changes
cargo/lib/dependabot/cargo/update_checker/version_resolver.rb Outdated
@@ -188,6 +185,7 @@ def handle_cargo_errors(error)
end

if error.message.include?("authenticate when downloading repo") ||
# TODO stop catching this 200 error: https://github.com/dependabot/dependabot-core/pull/5332#discussion_r936888624
Copy link
Member Author

Choose a reason for hiding this comment

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

Discovered this while looking at a related git blame... This could be pulled into a separate PR, but since just this small comment I thought okay to shoehorn into this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about we create a new draft PR that makes the change suggested by the TODO, and then we can pull that in when we're ready for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea, I'll open a draft PR shortly... I'd also like to leave in the code comment so that we don't lose track of it...

Copy link
Member Author

Choose a reason for hiding this comment

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

#5967

@jeffwidman jeffwidman force-pushed the fix-rubocop-freeze-warnings branch 3 times, most recently from 46efc29 to a77350a Compare October 11, 2022 05:08
@jeffwidman jeffwidman force-pushed the fix-rubocop-freeze-warnings branch 2 times, most recently from 9a0f9ee to feaacf9 Compare October 24, 2022 22:57
@jeffwidman jeffwidman changed the title Fix rubocop freeze warnings Fix rubocop redundant freeze warnings Oct 24, 2022
@jeffwidman jeffwidman marked this pull request as ready for review October 24, 2022 22:59
@jeffwidman jeffwidman requested a review from a team as a code owner October 24, 2022 22:59
mattt
mattt reviewed Oct 24, 2022
View reviewed changes
cargo/lib/dependabot/cargo/update_checker/version_resolver.rb
@@ -188,6 +185,7 @@ def handle_cargo_errors(error)
end

if error.message.include?("authenticate when downloading repo") ||
# TODO: stop catching this 200 error: https://github.com/dependabot/dependabot-core/pull/5332#discussion_r936888624
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this included by accident?

Copy link
Member Author

Choose a reason for hiding this comment

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

#5468 (comment)

@jeffwidman
Copy link
Member Author

jeffwidman commented Oct 24, 2022 •
edited

Merging once passes CI as this lint rule is super safe fix

@jeffwidman @mattt
Fix Rubocop redundant freeze warnings
fd64bac 
Rubocop is warning about things that are double frozen, ie `.freeze` is
redundant.

These warnings started showing up as part of the Ruby 3.1 upgrade, and
were excluded from that PR in order to minimize noise.

So this fixes them.

Co-authored-by: Mattt <mattt@github.com>
@jeffwidman jeffwidman merged commit 77d2c67 into dependabot:main Oct 25, 2022
@jeffwidman jeffwidman deleted the fix-rubocop-freeze-warnings branch October 25, 2022 00:10
@pavera pavera mentioned this pull request Oct 31, 2022
Merged
@jeffwidman jeffwidman mentioned this pull request Nov 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattt mattt mattt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jeffwidman @mattt