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

Remove final_blank_line style from Layout/TrailingBlankLines #5533

Conversation

koic
Copy link
Member

@koic koic commented Feb 1, 2018

Follow up of #5463.

In EnforcedStyle: final_newline (default), the following are checked.

  • Superflous trailing blank lines at end of file.
  • The presence of final newline "\ n".

The above requirements to be checked by this cop are satisfied.

Thus this PR removes supported final_blank_line style which is unclear useful opportunity. This will cause the SupportedStyles to be one, so the style configuration will be removed from this cop.
Also change the test codes to leave the test cases of EnforcedStyle: final_newline (default) and indent it again.

See the discussion in #5463 for the background of this change.


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).
  • 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.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run rake default or rake parallel. It executes all tests and RuboCop for itself, and generates the documentation.

Follow up of rubocop#5463.

In `EnforcedStyle: final_newline (default)`, the following are checked.

- Superflous trailing blank lines at end of file.
- The presence of final newline `"\ n"`.

The above requirements to be checked by this cop are satisfied.

Thus this PR removes supported `final_blank_line` style which is unclear
useful opportunity. This will cause the `SupportedStyles` to be one, so
the style configuration will be removed from this cop.
Also change the test codes to leave the test cases of `EnforcedStyle:
final_newline (default)` and indent it again.

See the discussion in rubocop#5463 for the background of this change.
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 2, 2018

I'm fine with this change, but before we go forward I'd like to first check in someone using the other style to get an idea why (https://github.com/search?p=2&q=EnforcedStyle%3A+final_blank_line&type=Code&utf8=%E2%9C%93).

@koic
Copy link
Member Author

koic commented Feb 3, 2018

That's exactly right.
I investigated .rubocop.yml using git blame in some repositories, but I could not find a definite change intention to EnforedStyle: final_blank_line.

I don't know the advantage of final_blank_line, but there may be some advantages to style of inserting blank line at end of line.

On the other hand, I confirmed the setting of .editorconfig during investigation.
https://github.com/bbatsov/rubocop/blob/v0.52.1/.editorconfig#L8-L9

The .editorconfig has trim_trailing_whitespace and insert_final_newline settings, which can be set like final_newline. But I could not find a setting highlighting the advantage of final_blank_line.

This is difficult to conclude. I don't know the advantage of final_blank_line. On the other hand, this conflicts with "Superflous trailing blank lines at end of file.", which is a role of final_newline.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 3, 2018

Seems I found the history of this change here - #490

@koic
Copy link
Member Author

koic commented Feb 3, 2018

Thank you for finding that discussion! 🙏
By leaving final_blank_line based on the historical background, it seemed that there was no confusion.
I think I will close this PR. WDYT?

@koic
Copy link
Member Author

koic commented Feb 3, 2018

[IMO] Of course, since the source code as the artifact is more important than an editor setting in a manufacturing process, I think that new_blank_line should not be inherently preferred. Apart from that, I understand that this alternative style is a solution to the above linked discussion 😃

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 4, 2018

Yeah, definitely. Perhaps we need to extend the documentation of the cop to elaborate on this and shed some light on the second configuration option.

@koic koic closed this Feb 4, 2018
@koic koic deleted the remove_final_blank_line_from_training_blank_line_cop branch February 4, 2018 12:52
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.

2 participants