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 #3937] Add new Rails/StartEndWith cop #3991

Merged
merged 1 commit into from Mar 6, 2017
Merged

[Fix #3937] Add new Rails/StartEndWith cop #3991

merged 1 commit into from Mar 6, 2017

Conversation

tdeo
Copy link
Contributor

@tdeo tdeo commented Jan 30, 2017

This cop checks for consistent use of starts_with? and ends_with? string
methods or consistent use of start_with? and end_with?

No option to mix consistent use of starts_with? and end_with? or the opposite.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Updated cop documentation with rake generate_cops_documentation (required only when you've added a new cop or changed the configuration/documentation of an existing cop).

@bbatsov
Copy link
Collaborator

bbatsov commented Jan 31, 2017

We should actually have a single cop checking for all similar ActiveSupport aliases.

@tdeo
Copy link
Contributor Author

tdeo commented Feb 13, 2017

Do you have any specific one in mind? Or how could we enumerate all such singular / plural forms in aliases?

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 14, 2017

Here's a listing of all the methods - http://guides.rubyonrails.org/active_support_core_extensions.html

@tdeo
Copy link
Contributor Author

tdeo commented Feb 14, 2017

There's indeed several other examples: String#camelize and String#titleizealiased respectively to String#camelcase and String#titlecase.

Otherwise there's also:

  • Array#append and Array#prepend
  • Hash#reverse_update, Hash#to_options and Hash#to_options!
  • Several Date and DateTime methods.
  • The Numeric#byte, Numeric#kilobyte, Numeric#megabyte, ... available in singular and plural forms (which I fell should belong in Rails/PluralizationGrammar)

I'm not sure if all of them should belong to the same cop (which would then be Rails/ActiveSupportAliases that can be enabled / disabled) ?

@rrosenblum
Copy link
Contributor

Aren't some of these aliases designed for grammatical purposes more so than as a replacement? In my mind this should work similar to what is being done in Rails/PluralizationGrammar. Rather than enforcing an always use one style, I would expect this to attempt to figure out if the receiver uses a singular or a plural name.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 16, 2017

Aren't some of these aliases designed for grammatical purposes more so than as a replacement?

They were, but this is pointless in a programming language. Matz himself mentioned this a while back when asked what he thinks about those.

@tdeo
Copy link
Contributor Author

tdeo commented Feb 16, 2017

So we would rather head towards a cop Rails/VanillaSemantics that would handle all of them at once?

People will still have a choice to enable or disable it anyway. But yeah, it would be "all or nothing".

@tdeo
Copy link
Contributor Author

tdeo commented Feb 21, 2017

@bbatsov I renamed the cop to Rails/ActiveSupportAliases to be consistent with #3935.

I'm only checking for String#starts_with?, String#ends_with?, Array#append and Array#prepend which are the only aliases to core ruby methods (the other I mentioned before are actually alias to other ActiveSupport methods).

A detail here: I'm not supporting autocorrect from Array#append to Array#<< because I didn't know how to handle removing the . for method call and the parameter parenthesis if present. I felt that autocorrecting to array.<<(element) was too ugly.

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@
* [#4018](https://github.com/bbatsov/rubocop/pull/4018): Add autocorrect `Lint/EmptyEnsure` cop. ([@pocke][])
* [#4028](https://github.com/bbatsov/rubocop/pull/4028): Add new `Style/IndentHeredoc` cop. ([@pocke][])
* Add new `Style/InverseMethods` cop. ([@rrosenblum][])
# [#3937](https://github.com/bbatsov/rubocop/issues/3937): Add new `Rails/ActiveSupportAliases` cop. ([@tdeo][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

You have a typo here - # should be *.

Description: >-
Avoid ActiveSupport aliases of standard ruby methods:
`String#starts_with?`, `String#ends_with?`,
`Array#append`, `Array#prepend`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This sentence should end with a ..

module Cop
module Rails
# This cop checks that ActiveSupport aliases to core ruby methods
# are not used
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add . here.

# 'some_string'.end_with?('suffix')
# [1, 2, 'a'] << 'b'
# [1, 2, 'a'].unshift('b')
# # bad
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a blank line between the code examples.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 3, 2017

A detail here: I'm not supporting autocorrect from Array#append to Array#<< because I didn't know how to handle removing the . for method call and the parameter parenthesis if present. I felt that autocorrecting to array.<<(element) was too ugly.

You can simply replace . with op in the autocorrect. It's accessible via node.loc as everything else.

I've added a few small remarks to the code. Btw, ideally this cop should be configurable to both enforce consistent non-usage of the aliases and their consistent usage. I'll merge the cop even in its current form, but I guess some people won't be happy about the lack of config options.

@tdeo
Copy link
Contributor Author

tdeo commented Mar 3, 2017

I still can't figure a good way for autocorrecting append that handles all those cases:

array.append(3)
array.append 3
array
  .append 3

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 3, 2017

Well, let's leave this out of the cop's scope for now.

@tdeo
Copy link
Contributor Author

tdeo commented Mar 3, 2017

And about different styles, I don't see any issue adding the option for consistent non-usage, but I think some people will still be unhappy having to write Array#append just because you want to use String#ends_with?.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 4, 2017

Fine by me. Just squash and rebase and I'll merge this.

This cop checks for use of ActiveSupport aliases to core ruby methods:
String#starts_with?, String#ends_with?, Array#append, Array#prepend
@bbatsov bbatsov merged commit 3d63895 into rubocop:master Mar 6, 2017
@tdeo tdeo deleted the start_end_with branch March 6, 2017 09:35
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

3 participants