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 option to autocorrect command to set indent width #1952

Closed
wants to merge 2 commits into from
Closed

Add option to autocorrect command to set indent width #1952

wants to merge 2 commits into from

Conversation

RubenSandwich
Copy link

This pull request adds an option to set the indent width when reformating code.

$ swiftlint  autocorrect --format --indent-width 2

This partially resolves the issue: #319, specifically the part related to:

Spacing: Indent using 2 spaces. Should be parametrizable, and probably 4 by default.

@SwiftLintBot
Copy link

SwiftLintBot commented Nov 17, 2017

12 Messages
📖 Linting Aerial with this PR took 0.45s vs 0.39s on master (15% slower)
📖 Linting Alamofire with this PR took 3.29s vs 3.29s on master (0% slower)
📖 Linting Firefox with this PR took 13.19s vs 13.22s on master (0% faster)
📖 Linting Kickstarter with this PR took 20.92s vs 20.65s on master (1% slower)
📖 Linting Moya with this PR took 1.87s vs 1.78s on master (5% slower)
📖 Linting Nimble with this PR took 1.97s vs 1.97s on master (0% slower)
📖 Linting Quick with this PR took 0.58s vs 0.56s on master (3% slower)
📖 Linting Realm with this PR took 3.62s vs 3.48s on master (4% slower)
📖 Linting SourceKitten with this PR took 1.13s vs 1.11s on master (1% slower)
📖 Linting Sourcery with this PR took 4.69s vs 4.64s on master (1% slower)
📖 Linting Swift with this PR took 15.29s vs 14.76s on master (3% slower)
📖 Linting WordPress with this PR took 13.25s vs 13.23s on master (0% slower)

Generated by 🚫 Danger

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@a03f8f8). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1952   +/-   ##
=========================================
  Coverage          ?   88.76%           
=========================================
  Files             ?      245           
  Lines             ?    11923           
  Branches          ?        0           
=========================================
  Hits              ?    10584           
  Misses            ?     1339           
  Partials          ?        0
Impacted Files Coverage Δ
Source/swiftlint/Commands/AutoCorrectCommand.swift 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a03f8f8...841c54a. Read the comment docs.

@jpsim
Copy link
Collaborator

jpsim commented Nov 17, 2017

I wonder if this should be a configuration value instead, paving the way to enforce indentation in linting in the future?

@RubenSandwich
Copy link
Author

@jpsim Oh I agree absolutely. Unfortunately, I didn't have enough time to look into the codebase on how to implement that as I'm going on vacation tomorrow so I started with this small change. (It also matches with replacing tabs with spaces and vice versa which to my research is limited to the autocorrect tool as well.)

@RubenSandwich
Copy link
Author

@jpsim Any update on this? I'd like to get this merged and I'll willing to do some extra work to make sure it does as I believe it would be helpful to others.

I think it is fine as is because my changes followed the pattern set by the useTabs settings, which also only exists in the autoCorrect tool, but I can understand wanting the indentWidth setting to be in the .swiftlint-ci.yml file.

However, if that happens I think these should also happen:

  1. Add indentWidth to rules
  2. Add useTabs to rules
  3. Emit warnings on lint for indentWidth and useTabs rules
  4. Use the indentWidth and useTabs rules in autocorrect if detected

Otherwise, I believe people will be confused if the useTabs or indentWidth settings are only used for one of the tools in the toolbox. So because of that, I think this work should be merged as a first step towards the goal of supporting all of those features.

@jpsim
Copy link
Collaborator

jpsim commented Dec 20, 2017

My thoughts on this haven't changed. This should be a configuration setting rather than a CLI flag.

Perhaps:

indentation: tabs
# or number of spaces
indentation: 4
# or
indentation: 2

We could introduce this in stages similar to what you described in your last comment:

  1. Introduce the configuration option
  2. Read it when running autocorrect --format
  3. Fix existing rules that don't behave well with tabs (e.g. vertical_parameter_alignment and others)
  4. Write new rules that can leverage indentation configurations

@jpsim jpsim mentioned this pull request Dec 29, 2017
@Jeehut
Copy link
Collaborator

Jeehut commented Feb 25, 2019

@RubenSandwich @jpsim Isn't this superseded by #1893 by now? We should close this if so.

@RubenSandwich
Copy link
Author

RubenSandwich commented Feb 25, 2019

@Dschee I believe it this was added in V2.41 . I don't write much Swift anymore so I haven't tried the recent versions of SwiftLint. I'm closing this pull request as I'm surprised it's still open, sorry about not cleaning it up earlier.

@Jeehut
Copy link
Collaborator

Jeehut commented Feb 25, 2019

Thanks @RubenSandwich, no worries! 👍

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.

5 participants