Allow option configuration for the Xcode Extension #216 #228

VinceBurn · 2018-02-23T17:55:48Z

The UI of the companion app use the Command Line argument possible values to give choice to the user.
FormatOptions is saved in UserDefaults using the values of command line argument.
The content of UserDefaults for Rules and Options can be save to a file and be loaded from a file.

There is a lot of code movement from the CommandLine.swift file to the Descriptor file.
The goal of this movement is to:

expose the transform of command line argument value to FormatOptions properties.
expose default value as command line argument value
expose free text validation rules

The item exposed are exposed in a way that can be process by the companion app to generalize the presentation of the different options.
They also allow a generalized way of saving and loading the options from UserDefaults.
That also facilitate the writing of targeted unit test for the different transforms.

Move RuleSelectionTableCellView to MainBinarySelectionTableCellView Update view model to work with UserSelection Concept

id will probably be used as the key for saving the item in UserDefaults

Default value should match input from the command line

Save the config to a file Open (load) a config from a file Reset the config to the defaults values

VinceBurn · 2018-03-06T02:31:12Z

Hi @nicklockwood , I've add a save to file option in the companion app using Codable for simplicity.
I know it is a big merge request.

nicklockwood

@VinceBurn wow, this is amazing thank you. There's a lot here so it's hard for me to review it in detail, but I've made a first pass of comments and I may have more later.

@tonyarnold if you have time (and inclination) it would be great to get your thoughts.

I think this will need a few iterations before I'm happy to release it, but I'll try to get it landed in the develop branch as soon as possible so you aren't stuck in an endless rebasing loop. We can always make further changes once it's landed.

nicklockwood · 2018-03-07T16:58:08Z

EditorExtension/Application/Source/AppDelegate.swift

+}
+
+struct SwiftFormatXcodeConfiguration: Codable {
+    static let fileExtension = "sfxx" //  TODO: Define the official extension


I think if we're going to have a import/export file format then it should be a human-readable format that is suitable for use with the CLI as well.

I've been resistant to doing that because it doesn't add a huge amount of value vs using a .sh file, but as a way to share config between the CLI and the extension it makes more sense.

I don't really like JSON for config, but I'm reluctant to add a dependency for a YAML parser or whatever. I'll probably just implement a basic TOML or INI parser. I'll do that in parallel while we work on getting this PR ready.

I'm not attached to JSON, I made the objects conform to Codable thinking that we could write a different Coder / Decoder later if we felt the need for it, I haven't check how much work it would be.

The thing I would like to keep is a custom fileExtension so I could leave the configuration at the root of a project and double click on it to have it open the application and load the configuration. That functionality is not part of this PR.

nicklockwood · 2018-03-07T16:58:50Z

EditorExtension/Application/Source/AppDelegate.swift

@@ -31,5 +31,137 @@

 import Cocoa

+struct Version: Codable {


Is AppDelegate the best place for this stuff?

Effectively it is no the place. I was thinking about a refactoring that would put the handling of the file data outside of the AppDelegate.

nicklockwood · 2018-03-07T17:00:30Z

EditorExtension/Application/Source/AppDelegate.swift

+    let options: [SavedOption]
+}
+
+enum XcodeConfigurationError: Error, CustomStringConvertible, EnumAssociatable {


This seems very similar to the ParsingError type I already have. Can't we just reuse that?

Yes, will do. I don't recall why I didn't go that way.

nicklockwood · 2018-03-07T17:02:49Z

EditorExtension/Application/Source/HeaderTableCellView.swift

+
+import Cocoa
+
+class HeaderTableCellView: NSTableCellView {


I think it would be better to have options in a 3rd tab, rather than have two subsections in the rules tab. The currently layout involves a lot of scrolling, and the dark grey headings don't look very idiomatic for a Mac app (at least to me).

Since Rules and Options are closely related, my end goal would be to have the options that affect a single rules group with the impacted rule.
I was thinking of going that way in a future improvement, that is why I didn't put it in a different tab.

Since that's unlikely to ship in the next release, I think it's better to go for the more user-friendly design of having three separate tabs.

I would like to rethink the rules/options relationship at some point though, because it's confusing as it stands currently.

For rules/options relationship, (correct me if I'm wrong) but options feel like argument values passed to rules. Options look like they have a 'to one rule' relationship, except for a few one that have a 'to many rules' relationship.
with that in mind I was thinking of having the 'to one rule' UI look something like this:

Rule Name

___ Option Name

___ Option Name

But I'm not sure about how to play the 'to many rules' options. I have different paths in my mind right now.

Have a 'to many rules options' section that would be like the current option section (a different section at the end)

Have the 'to many rules options' show under every Rule they are affecting and keeping all those UI in sync when user change one of those

Have a separate tabs for the 'to many rules options'.

Break 'to many rules options' to multiple options that impact a single rules at a time.

I haven't done a formal cataloging of those relationship yet to see the numbers of 'to one' and 'to many' options.

I would be attacking this issus as my next contribution, so going extra tab for this release and going back to a unify tab in a few weeks would feel like doing a work for a very temporary patch, and I have the feeling that the current implementation is "good enough" and that splitting it on 2 tabs would not bring a big enough avantage to fo that way for a few weeks.

@VinceBurn that seems like an accurate description.

I think it's possible that the options that apply to multiple rules make actually be logically associated with one particular rule, it's just that other rules may need to take that option into account.

For example: the --linebreaks option is logically associated with the linebreaks rule, but in practice there are several other rules (e.g, the braces or wrapArguments rules that may need to insert linebreaks, so they need to read that option.

So perhaps a solution would be to group the options inside the metadata for the rules that they most directly relate to, and have the other rules access those options via the another rule's namespace.

@nicklockwood an other way of seeing this, is to consider that certain rule need to re-apply other rules when they perform some action.
But this is not totally accurate, because a rule could be disabled and an other rule would use its associated option to perform some action.
So the user should have a way to know that a rule may be impacted by an option that is primarily associated with an other rule... Because if the UI associate such an option to only one rule, it would be misleading for the user, he would assume that by disabling the associated rule he doesn't need to provide a value for the option.

With all those consideration I think it means that we would need to go with option
2. Have the 'to many rules options' show under every Rule they are affecting and keeping all those UI in sync when user change one of those
5. (some other creative way we haven't figure out yet)

Back to the original topic:
Are you confortable with keeping the rules & options in the same tab for now, knowing that improving this tab will be my next focus?

nicklockwood · 2018-03-07T17:04:27Z

EditorExtension/Application/Source/RulesViewController.swift

-        return tableView.makeView(withIdentifier: .ruleSelectionTableCellView, owner: self) as? RuleSelectionTableCellView
+    func tableView(_ tableView: NSTableView, viewFor _: NSTableColumn?, row: Int) -> NSView? {
+        let model = self.model(forRow: row)
+        let ID: NSUserInterfaceItemIdentifier


lowercase "id" please.

VinceBurn · 2018-03-07T17:56:10Z

Sources/OptionsDescriptor.swift

+    static let allowInlineSemicolons = FormatOptions.Descriptor(id: "allow-inline-semicolons",
+                                                                argumentName: "semicolons",
+                                                                propertyName: "allowInlineSemicolons",
+                                                                name: "allowInlineSemicolons",


For name I've inconsistently used name resembling the argument name or the property name. I've been using the application a little bit in the last days and I find that confusing.
@nicklockwood what would you think if I consistently use the argumentName for name.

@VinceBurn I think it would definitely be better to use the same name everywhere. We can always look at setting up a map to more readable names later, but until then, having fewer different names for the same thing is less confusing.

@nicklockwood what if we always base everything on the argument name with the following convention?

argumentName: removelines

name: Remove Lines (separate word by space and capitalize)

id: remove-lines (separate word by dash all lower case)

@nicklockwood I've made change to option names and ids. In those changes id always follow argument name, but with '-' between words. Name is almost always argument name Capitalized with space. On few occasion there is a word added to be more precise.
If you had saved a file before, trying to load it will crash the app.

VinceBurn · 2018-03-09T03:20:13Z

Sources/OptionsStore.swift

+
+    fileprivate init(_ rep: OptionsStore.OptionRepresentation) {
+        argumentValue = rep.arg
+        //  FIXME, No force unwrap here, throw instead


Don't forget to fix this, to show a message to the user instead of crashing if the file has some strange value.

tonyarnold · 2018-03-09T12:05:52Z

This looks fantastic, and my comments would basically mirror @nicklockwood's - the options and rules should be in separate tabs.

One thing I would like to look at before we release this is converting the options to use an NSGridView rather than an NSTableView for a much more consistent layout for this large list of items. I'm happy to explore that once your work is merged @VinceBurn - it may not pan out, but I think it could be really nice.

Thanks again for putting so much time into this!

VinceBurn · 2018-03-09T13:45:04Z

Thanks @tonyarnold , I would like to put options that impact a rule with that rule in the UI in a future improvement (there is a conversation above for that aspect).
We should coordinate our effort on that point to avoid extra work. After this PR is accepted I will create an issue to promote the discussion on that point. ( I was waiting because the improvement issue make no sense if the current PR is not in)

VinceBurn · 2018-03-30T13:11:26Z

@nicklockwood I just had a flash for the file format, what do you think if the file is just command line arguments?

ex.

--rules blankLinesAtEndOfScope,blankLinesBetweenScopes,braces,consecutiveBlankLines,consecutiveSpaces,duplicateImports,elseOnSameLine,hoistPatternLet,indent,linebreakAtEndOfFile,linebreaks,numberFormatting,ranges,redundantBackticks,redundantGet,redundantInit,redundantLet,redundantNilInit,redundantParens,redundantPattern,redundantSelf,redundantVoidReturnType,semicolons,spaceAroundBraces,spaceAroundBrackets,spaceAroundComments,spaceAroundGenerics,spaceAroundOperators,spaceAroundParens,spaceInsideBraces,spaceInsideBrackets,spaceInsideComments,spaceInsideGenerics,spaceInsideParens,specifiers,todos,trailingCommas,trailingSpace,void,wrapArguments,sortedImports,unusedArguments --allman false --self remove --header ignore --hexgrouping 4,8 --hexliteralcase uppercase --commas always --patternlet inline --exponentcase lowercase --indentcase false --binarygrouping 4,8 --semicolons inline --decimalgrouping 3,6 --elseposition next-line --experimental disabled --operatorfunc space --trimwhitespace always --indent 4 --octalgrouping 4,8 --wraparguments afterfirst --stripunusedargs closure-only --wrapelements beforefirst --empty void --ranges space --linebreaks lf --comments indent --ifdef indent

(we should add new lines char before each --)
That way it would be very easy to share arguments with the command line

swiftformat ./  --argfile ./formatConfig

And it would be convenient to allow people to add command line specific arguments to the file like --exclude ./Pods,./ThirdParty and just ignore them when loading for the extension.

tonyarnold · 2018-07-25T23:09:45Z

Should the new options be showing up in the UI? I know it has been a while, but I'm only seeing the rules, and not the full list that I see in #216.

Oh right, it's merged into develop - my bad!

nicklockwood · 2018-07-26T05:59:59Z

@tonyarnold yes, sorry, I’m still tinkering with it a bit, but it shouldn’t be too long until it ships now.

tonyarnold · 2018-07-26T06:06:59Z

No need to apologise! I just built my own copy - it's looking really good 😎

nicklockwood · 2019-02-07T00:51:11Z

@VinceBurn I don't know if you're still interested in pursuing this, but I just added options metadata to the rules (each rule now has an array of options that it uses) which would make it possible to implement an extension UI more like you originally envisioned, instead of rules and options being in separate tabs.

VinceBurn · 2019-02-07T14:52:54Z

Hi @nicklockwood,
I'm still interested, do you have an issue for this where we could discuss possible end results?

nicklockwood · 2019-02-07T21:11:57Z

@VinceBurn great! I've opened an issue here.

VinceBurn added 30 commits February 21, 2018 18:28

UI Exploration

c03039d

Add UserSelection Concept

b15140e

Move RuleSelectionTableCellView to MainBinarySelectionTableCellView Update view model to work with UserSelection Concept

Add Header Support

9a64b8c

Add List Selection Support

1a02444

Add FreeTextTableCellView support

7303ffb

List option exploration with LineBreaks

85e6a23

Binary option exploration with useVoid

d5a9d6c

Iterating over Option that have descriptor

328bcde

Add id for descriptor

7c6977f

id will probably be used as the key for saving the item in UserDefaults

Move static descriptor into the Descriptor struct + add id

c356e8d

Descriptor.default: String instead of Any

4284106

Default value should match input from the command line

Question to self

90e4475

rename to emphasis that we rely on command line argument values

92f082a

Add FreeText type

0115cc9

Use decimalGrouping in companion app UI

057c70b

swift format

a7a5501

Move Options Descriptor to its own file

2c42b08

Add Test File

c35e898

Add EnumAssociatable protocol

4344e1b

Add EnumAssociatable implementation

5ea41bf

ArgumentType as EnumAssociatable

334ed68

Test useVoid Descriptor

f21ff13

Test Linebreak-character Descriptor

bae9ff9

EnumAssociatable document limitation in code

974a519

Test decimal-grouping Descriptor

171a90e

REFACTOR Identifier Properties

ff81fb2

REFACTOR validateSutThrowFormatErrorOptions

7621e83

allowInlineSemicolons Descriptor + Test Refactoring

44ee5e9

REFACTOR binary type test to be more compact

2f3afae

indefIndent Descriptor

1557a8d

VinceBurn added 3 commits March 5, 2018 20:46

Add Save, Open, Reset

f64ad4c

Save the config to a file Open (load) a config from a file Reset the config to the defaults values

Add deprecation message

fc95fc6

remove leftover useless print

c33299e

nicklockwood reviewed Mar 7, 2018

View reviewed changes

lowercase id

f2143ee

VinceBurn commented Mar 7, 2018

View reviewed changes

VinceBurn added 9 commits March 7, 2018 22:15

Use FormatError instead of XcodeConfigurationError

43b302f

Rename SwiftFormatFile

7863147

Move encode and decode code to the SwiftFormatFile

24daa11

SwiftFormatFile in its own file

2e5c730

Move extension to end of file

25e748b

Update test for added file

c489102

Fix Test Requirement message

d0a8ef9

Modification of Options names and ids

29fea92

Dev note

fc003a8

VinceBurn commented Mar 9, 2018

View reviewed changes

VinceBurn added 2 commits April 12, 2018 19:06

Add descriptif error when Saved Option ID doesn't match known ID

b485ce2

New File Format closer to CLI arguments

265be2d

nicklockwood merged commit 42d61e1 into nicklockwood:develop Jul 5, 2018

nicklockwood mentioned this pull request Feb 7, 2019

Group options with their associated rules in SwiftFormat for Xcode app #353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow option configuration for the Xcode Extension #216 #228

Allow option configuration for the Xcode Extension #216 #228

VinceBurn commented Feb 23, 2018 •

edited

VinceBurn commented Mar 6, 2018

nicklockwood left a comment

nicklockwood Mar 7, 2018

VinceBurn Mar 7, 2018

nicklockwood Mar 7, 2018

VinceBurn Mar 7, 2018

nicklockwood Mar 7, 2018

VinceBurn Mar 7, 2018

nicklockwood Mar 7, 2018

VinceBurn Mar 7, 2018

nicklockwood Mar 8, 2018

VinceBurn Mar 8, 2018 •

edited

nicklockwood Mar 8, 2018

VinceBurn Mar 8, 2018

nicklockwood Mar 7, 2018

VinceBurn Mar 7, 2018

nicklockwood Mar 8, 2018

VinceBurn Mar 8, 2018

VinceBurn Mar 9, 2018

VinceBurn Mar 9, 2018

tonyarnold commented Mar 9, 2018

VinceBurn commented Mar 9, 2018

VinceBurn commented Mar 30, 2018 •

edited

tonyarnold commented Jul 25, 2018 •

edited

nicklockwood commented Jul 26, 2018

tonyarnold commented Jul 26, 2018 •

edited

nicklockwood commented Feb 7, 2019 •

edited

VinceBurn commented Feb 7, 2019

nicklockwood commented Feb 7, 2019

Allow option configuration for the Xcode Extension #216 #228

Allow option configuration for the Xcode Extension #216 #228

Conversation

VinceBurn commented Feb 23, 2018 • edited

VinceBurn commented Mar 6, 2018

nicklockwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VinceBurn Mar 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonyarnold commented Mar 9, 2018

VinceBurn commented Mar 9, 2018

VinceBurn commented Mar 30, 2018 • edited

tonyarnold commented Jul 25, 2018 • edited

nicklockwood commented Jul 26, 2018

tonyarnold commented Jul 26, 2018 • edited

nicklockwood commented Feb 7, 2019 • edited

VinceBurn commented Feb 7, 2019

nicklockwood commented Feb 7, 2019

VinceBurn commented Feb 23, 2018 •

edited

VinceBurn Mar 8, 2018 •

edited

VinceBurn commented Mar 30, 2018 •

edited

tonyarnold commented Jul 25, 2018 •

edited

tonyarnold commented Jul 26, 2018 •

edited

nicklockwood commented Feb 7, 2019 •

edited