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

Add Style/EmptyLineAfterMagicComment cop #3889

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

backus
Copy link
Contributor

@backus backus commented Jan 10, 2017

Depends on rubocop/ruby-style-guide#616


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).

@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch 2 times, most recently from 4f08fed to b6d72e9 Compare January 10, 2017 09:47
@backus backus changed the title Add Style/EmptyLineAfterMagicComment cop [WIP] Add Style/EmptyLineAfterMagicComment cop Jan 10, 2017
@backus
Copy link
Contributor Author

backus commented Jan 10, 2017

Looks like this touches more tests than I thought. Work in progress....

end

it 'accepts code that separates the comment from the code with a newline' do
inspect_source(cop, ["# frozen_string_literal: true\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

What are your thoughts on writing this as

inspect_source(cop, ["# frozen_string_literal: true",
                     '',
                     'class Foo; end'])

In my mind, it is a little clearer that there is an empty line between the comment and the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm fine with either just didn't know what was the convention

@rrosenblum
Copy link
Contributor

Should this be configurable so that the user can choose whether they want to enforce having an empty line or not having an empty line?

I don't think that I saw a test that includes a magic comment and documentation comment. Should the following code register an offense?

# frozen_string_literal: true
# Class documentation
class Foo
end

@Drenmi
Copy link
Collaborator

Drenmi commented Jan 11, 2017

I don't think that I saw a test that includes a magic comment and documentation comment. Should the following code register an offense?

# frozen_string_literal: true
# Class documentation
class Foo
end

IMHO, it should. To me, the lack of spacing would indicate that both comment lines pertain to Foo, which is not the case, here.

@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch from b6d72e9 to 02b7f47 Compare January 11, 2017 09:24
@backus
Copy link
Contributor Author

backus commented Jan 11, 2017

Yeah good catch guys I also need to add a test case for separating the frozen_string_literal comment from the class documentation

@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch 3 times, most recently from 6195698 to 06c977d Compare January 23, 2017 01:55
@backus backus changed the title [WIP] Add Style/EmptyLineAfterMagicComment cop Add Style/EmptyLineAfterMagicComment cop Jan 23, 2017
@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch from 06c977d to 96fd191 Compare January 23, 2017 02:05
@backus
Copy link
Contributor Author

backus commented Jan 23, 2017

@bbatsov build is passing finally minus this weird jruby bug that is plaguing every build.

@backus
Copy link
Contributor Author

backus commented Jan 23, 2017

@Drenmi and @rrosenblum I think I addressed your concerns

@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch from 96fd191 to 3f36f24 Compare January 24, 2017 10:21
@backus backus force-pushed the feature/EmptyLineAfterMagicComment branch from 3f36f24 to e17fecf Compare January 24, 2017 10:25
@bbatsov bbatsov merged commit 1f59c20 into rubocop:master Jan 25, 2017
@backus backus deleted the feature/EmptyLineAfterMagicComment branch January 25, 2017 03:44
dennmart added a commit to ableco/ablecop that referenced this pull request Apr 5, 2017
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

4 participants