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 whitespaceOnly option to configuration file #195

Open
wants to merge 1 commit into
base: swift-5.2-branch
Choose a base branch
from

Conversation

zmeyc
Copy link

@zmeyc zmeyc commented May 18, 2020

Allows configuring whether formatter should add or remove trailing commas in lists.

Allow configuring whether formatter should add or remove
trailing commas in lists.
@allevato
Copy link
Collaborator

Thanks for the contribution!

I don't think we want to call a user-configurable trailing comma feature something as general as "whitespace only". We do that in the internal implementation because the linter half of the pretty printer works by looking at runs of whitespace but expects the non-whitespace characters to be the same, and the formatted output for the linter is never otherwise used, but we don't have to (and shouldn't) make the user-facing feature work the same way. If we ever add other situations in the pretty printer where it inserts or removes non-whitespace characters (as a hypothetical, we could do something like add parentheses around long wrapped expressions), we don't want a single setting to turn all of those features on/off.

Instead, let's call it multilineCollectionTrailingCommas. To make it fully configurable, its value should be a tri-state instead of a Boolean, because there are three possible choices:

  • always: Always require a trailing comma, inserting one if the user didn't have one originally—with the exception that we never do for single-element collections, to avoid awkward situations where array/dictionary sugar is used as a type but in expression context, because we don't want this:

    let array = [
      SomeLongElementTypeName,
    ]()
    
  • never: Never allow a trailing comma, removing one if the user had one there.

  • ignore: Do nothing; leave whatever the user had there, whether the trailing comma was present or not.

This concept is general enough that I can see it applying to future configuration settings, so I think defining a general enum with these three cases would be best rather than tying it specifically to trailing commas. That will help us define a common language for configuration going forward, too.

With that change, you'll need to make the actual change on the pretty-printer side in TokenStreamCreator instead of PrettyPrinter, because you'll have to make a decision about which formatting tokens you insert or which real tokens you skip based on that configuration setting.


Oh, and please make sure your PR is set to merge into master, not into the swift-5.2-branch when it's ready—I know it can be tricky to test against since it's pinned against dev snapshots (and I can help you verify it), but we only merge new changes into master and cherry-pick them into the other release branches.

@zmeyc
Copy link
Author

zmeyc commented May 18, 2020

Possibly even more flexible format is needed, something similar to ESLint's:
https://eslint.org/docs/rules/comma-style

.eslintrc.json
{
    "comma-dangle": ["error", "never"],
    // or
    "comma-dangle": ["error", {
        "arrays": "only-multiline",
        "objects": "never",
        "imports": "never",
        "exports": "never",
        "functions": "never"
    }]
}

ESLint's first parameter is whether the rule results in an error or in a warning, other parameters are rule-specific. Some eslint rules (import sort order etc) require relatively complex configuration.

@allevato
Copy link
Collaborator

All lint violations are emitted at the warning level right now, and that's hardcoded. If we want to support different severity levels for different kinds of violations in the future (and we might!), that's an independent change from what we do here for trailing commas today and would be a much larger overhaul of our configuration format, so for the scope of this PR, we should stay focused on the trailing comma issue.

Swift has fewer trailing comma contexts that apply than Javascript/Typescript—only array and dictionary literals. I personally don't see the need to make those two things separately configurable (other folks may disagree), so in the interest of avoiding feature creep, I think it's better to start with just making the feature apply to both arrays and dictionaries in the same way.

If we do need a more complex configuration sub-object in the future (for example, if we determined there was a need to format trailing commas for arrays and dictionaries differently), then that's fairly straightforward to do without breaking users: the custom implementation of Configuration.init(decoder:) can just try decoding the field as multiple types until it finds one that is compatible, or it falls back to the default value.

aaditya-chandrasekhar pushed a commit to val-verde/swift-format that referenced this pull request May 20, 2021
…e#195)

* Fix public extensions exposing nested code of all access levels

* Inline access checking closure

Co-authored-by: Max Desiatov <max@desiatov.com>

* Update Changelog.md

* Update Changelog.md

* Add testComputedPropertiesInPublicExtension

* Add testComputedPropertiesWithMultipleAccessModifiersInPublicExtension

* Do not skip properties which have limited access of setters only

* Add missing case and fix incorrect checks in access level tests

Co-authored-by: Max Desiatov <max@desiatov.com>
Co-authored-by: Mattt <mattt@me.com>
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

2 participants