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 support for tabs in indentation. #7867

Merged
merged 7 commits into from Apr 15, 2020
Merged

Conversation

DracoAter
Copy link
Contributor

@DracoAter DracoAter commented Apr 11, 2020

As a stubborn tab user, I would like to have the ability to check, if all my code is indented with tabs, and also automatically correct indentation from spaces to tabs. So I:

Add support for indentation with tabs. Supports auto-correction.
The existing Layout/Tab is renamed into Layout/IndentationStyle.

Layout/IndentationStyle now can be configured:

# SupportedStyles: spaces, tabs
Layout/IndentationStyle:
  EnforcedStyle: spaces # default

EnforcedStyle: spaces will look for tabs in indentation and correct them into spaces.
EnforcedStyle: tabs will look for spaces in indentation and correct them into tabs.

Previous functionality of Layout/Tab that looked for tabs in places other than indentation is removed.
Hopefully this issue will be covered by TrailingWhitespace cop.

@DracoAter DracoAter changed the title Add support for tabs indentation. Add support for tabs in indentation. Apr 11, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 11, 2020

The proposed change is fine by me. Just a couple of notes:

  • I think "tabs" and "spaces" make for better style names
  • We'll probably have to rename this cop, as its current name is a bit confusing after the addition of the configuration

@DracoAter
Copy link
Contributor Author

I have fixed the linting issues and changed SupportedStyles to plural (spaces, tabs).

Concerning the renaming, of the cop, that would be a major version update then. Probably it is possible to get away with minor version update, if we split it into 2 different cops. Existing Layout/Tab will search and fix the tabs outside of indentation, and a new cop (e.g. Layout/IndentationStyle will look for and fix tabs/spaces only in indentation.

If you need, I can implement any strategy you suggest.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 11, 2020

Concerning the renaming, of the cop, that would be a major version update then.

We still haven't released 1.0, so that's not a big deal. I doubt the rename would impact many people, anyways, as it's not common to disable this cop.

Existing Layout/Tab will search and fix the tabs outside of indentation, and a new cop (e.g. Layout/IndentationStyle will look for and fix tabs/spaces only in indentation.

I do like this approach, though, as it's a bit weird that now the cop cares about both indentation and trailing tabs. I don't recall if it detects tabs in other contexts. I'm thinking that in essence the second cop can be something like "TrailingWhitespace" or something.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 11, 2020

Actually, we do have a trailing whitespace cop already, so unless there are other contexts in which a tab shouldn't appear I guess this check can be folded in the existing cop.

@koic
Copy link
Member

koic commented Apr 11, 2020

IMHO, I think the cop name can be left as Layout/Tab by using the option names below.

  • Rename spaces to soft_tabs
  • Rename tabs to hard_tabs

Of course it could be changed to a more suitable cop name or better approach.

@DracoAter
Copy link
Contributor Author

Currently it checks for tabs anywhere. Does not matter where the tab is (except inside a string), it is replaced with number of indentation_width spaces, even in the middle of the line.

@DracoAter
Copy link
Contributor Author

So what is your final decision, what do I need to do now: rename, split or nothing?

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 13, 2020

Let's go with the new name IndentationStyle or UnitsOfIndentation and drop the check for tabs that are not used for indentation purposes. If someone can come up with a better name - that'd be fine by me.

I don't see much point in a cop checking for tabs at random place in the file. Hopefully the TrailingWhitespace cop already addresses trailing tabs.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 14, 2020

Don't forget to mention this breaking change in the changelog and add the cop to the list of renamed/removed cops.

StyleGuide: '#spaces-indentation'
Enabled: true
VersionAdded: '0.49'
VersionChanged: '0.51'
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll have to changed this to 0.82.

@@ -0,0 +1,9 @@
### New features

* [#7867](https://github.com/rubocop-hq/rubocop/issues/7867): Add support for tabs in indentation. ([@DracoAter][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is auto-generated on release from the changelog. You shouldn't update it manually.

CHANGELOG.md Outdated
* [#7850](https://github.com/rubocop-hq/rubocop/issues/7850): Make it possible to enable/disable pending cops. ([@koic][])

### Changes

* Renamed `Layout/Tab` cop to `Layout/IndentationStyle` ([@DracoAter][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a **(Breaking)** prefix here.

CHANGELOG.md Outdated Show resolved Hide resolved
@bbatsov bbatsov merged commit 6b9510d into rubocop:master Apr 15, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented Apr 15, 2020

Thanks for working on this! I might not be fan of tabs, but I'm happy we'll be more accommodating to the people who are! :-)

@DracoAter
Copy link
Contributor Author

Thank you for the collaboration and quick replies.

@DracoAter
Copy link
Contributor Author

I screwed up url in changelog, could you fix it?

[@DracoAter]: https:/github.com/DracoAter

Add one more slash before github.

@bbatsov
Copy link
Collaborator

bbatsov commented Apr 15, 2020

Sure.

ipmsteven added a commit to ipmsteven/rubocop-github that referenced this pull request Apr 18, 2020
ipmsteven added a commit to ipmsteven/rubocop-github that referenced this pull request Apr 18, 2020
knappe added a commit to knappe/rubocop that referenced this pull request May 8, 2020
In rubocop#7867 a breaking change was introduced, but not linked in the Change log.  It would be nice to not have to track down which change was introduced and instead link it, following the convention of the other breaking entries.
bbatsov pushed a commit that referenced this pull request May 8, 2020
In #7867 a breaking change was introduced, but not linked in the Change log.  It would be nice to not have to track down which change was introduced and instead link it, following the convention of the other breaking entries.
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