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

Mark Style/FrozenStringLiteralComment as safe to autocorrect #199

Merged
merged 1 commit into from Sep 28, 2020
Merged

Mark Style/FrozenStringLiteralComment as safe to autocorrect #199

merged 1 commit into from Sep 28, 2020

Conversation

KaanOzkan
Copy link
Contributor

@KaanOzkan KaanOzkan commented Sep 28, 2020 •
edited

There was internal discussion regarding FrozenStringLiteralComment cop being deemed unsafe to autocorrect in Rubocop. This recent change threw people off that expect -a to fix FrozenStringLiteralComment errors as it has done in the past, eg. Shopify/rubocop-sorbet#46.

This past behaviour along with the assumption that we expect developers to write code that is frozen string safe lead to the decision of keeping the existing practice of -a autocorrecting this cop, even though it was marked as unsafe to autocorrect in rubocop.

Rubocop PRs that made autocorrect unsafe: rubocop/rubocop#7307 & rubocop/rubocop#8529

@KaanOzkan
Mark Style/FrozenStringLiteralComment as safe to autocorrect
55e6e59 
Upon internal discussion it was decided that we want to keep the
existing practice that `-a` autocorrects this cop, even though it was
marked as unsafe in rubocop.
@volmer
Copy link
Contributor

volmer commented Sep 28, 2020

I might be nice to include all the context of this change in the PR description, since not everyone involved in this project is part of Shopify or was included in the internal discussions mentioned above 🙂

@KaanOzkan KaanOzkan merged commit 0aa9a52 into master Sep 28, 2020
@KaanOzkan KaanOzkan deleted the ko-mark-frozen-comment-safe-ac branch September 28, 2020 19:57
@paracycle
Copy link
Member

Did this PR actually update the correct cop? rubocop/rubocop#8529 talks about Lint/FrozenStringLiteralComment being marked as SafeAutoCorrect: false but this PR has updated Style/FrozenStringLiteralComment. Are they equivalent?

@KaanOzkan
Copy link
Contributor Author

KaanOzkan commented Oct 1, 2020 •
edited

I'm not sure why the title of the PR is Lint/. The changed files changes Style/FrozenStringLiteralComment, https://github.com/rubocop-hq/rubocop/blob/master/config/default.yml#L3112

And I can only find a single FrozenStringLiteralComment: https://github.com/rubocop-hq/rubocop/blob/master/lib/rubocop/cop/style/frozen_string_literal_comment.rb

I haven't tested this but I don't think Lint/FrozenStringLiteralComment exists.

@paracycle
Copy link
Member

@KaanOzkan thanks for the clarification. I was going mad looking for a Lint/FrozenStringLiteralComment cop. All good, sorry for the noise. 👍

@andyw8
Copy link
Contributor

andyw8 commented Oct 1, 2020 •
edited

I think the RuboCop changelog and rubocop/rubocop#8529 are mistaken, since it always been a Style/ cop since being added in rubocop/rubocop#2542

It's been fixed: rubocop/rubocop@f845314

@volmer volmer temporarily deployed to rubygems October 15, 2020 13:48 Inactive
@sambostock sambostock mentioned this pull request Nov 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rafaelfranca rafaelfranca rafaelfranca approved these changes

@volmer volmer volmer approved these changes

@lastgabs lastgabs lastgabs approved these changes

@paracycle paracycle Awaiting requested review from paracycle

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
6 participants
@KaanOzkan @volmer @paracycle @andyw8 @rafaelfranca @lastgabs