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 #9734] Add new Style/TopLevelMethodDefinitions cop #9755

Merged
merged 1 commit into from May 7, 2021

Conversation

tejasbubane
Copy link
Contributor

@tejasbubane tejasbubane commented May 1, 2021

Closes #9734


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.

@tejasbubane tejasbubane force-pushed the fix-9734 branch 2 times, most recently from baba60e to aa1488f Compare May 1, 2021 19:29
config/default.yml Outdated Show resolved Hide resolved
@bbatsov
Copy link
Collaborator

bbatsov commented May 2, 2021

I think you can also use define_method at the top-level, which such a cop should probably detect.

@tejasbubane tejasbubane changed the title [Fix #9734] Add new Style/GlobalMethod cop [Fix #9734] Add new Style/TopLevelMethodDefinitions cop May 2, 2021
@tejasbubane tejasbubane force-pushed the fix-9734 branch 3 times, most recently from c1294d7 to 4dedd2a Compare May 2, 2021 19:39
@tejasbubane
Copy link
Contributor Author

@bbatsov @koic Changed as per your comments.

@tejasbubane tejasbubane requested review from koic and bbatsov May 2, 2021 19:58
@dvandersluis
Copy link
Member

I probably wouldn't want these to be allowed anyways, but I could see an argument for allowing a defs node where the receiver isn't self, since in that case, the method will end up scoped to an object and not be global.

ie.

def MyClass.foo
  # ...
end

I'm fine with this being disallowed but anyone have an opinion there?

@koic
Copy link
Member

koic commented May 3, 2021

I think it can be allowed if a receiver is explicitly specified. I think top-level methods that are implicitly declared directly to Object without specifying a receiver should be prevented.

@@ -4632,6 +4632,11 @@ Style/TernaryParentheses:
- require_parentheses_when_complex
AllowSafeAssignment: true

Style/TopLevelMethodDefinitions:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use singular here (Definition) to be aligned with how most of the cops are named.

@bbatsov
Copy link
Collaborator

bbatsov commented May 3, 2021

You forgot to update the commit message itself.

Btw, I'm thinking that it would be hard to add some check whether the file being analyzed is a script (if it has a shebang) and disable to the cop just for scripts, but probably that's an overkill at this point.

@bbatsov
Copy link
Collaborator

bbatsov commented May 3, 2021

I guess we should also document this in the Ruby style guide. I'm surprised we haven't done so already.

tejasbubane added a commit to tejasbubane/ruby-style-guide that referenced this pull request May 5, 2021
@tejasbubane
Copy link
Contributor Author

@bbatsov Changed to singular and created PR for styleguide.

@bbatsov bbatsov merged commit 928faeb into rubocop:master May 7, 2021
@bbatsov
Copy link
Collaborator

bbatsov commented May 7, 2021

Thanks!

bbatsov pushed a commit to rubocop/ruby-style-guide that referenced this pull request May 7, 2021
@tejasbubane tejasbubane deleted the fix-9734 branch May 7, 2021 07:15
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.

add cop for "global method"
4 participants