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

Format Eidolon using Prettier Swift plugin #1

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@sirlantis
Owner

sirlantis commented Jan 4, 2018

@sirlantis

The only real blunder to me is the missing space in switch foo{ - rest looks pretty solid. What do you think, @azz @vjeux?

Wondering whether @ashfurrow and @orta would approve of this formatting or whether this is all blasphemy. 🤐

}
cardHandler.cardStatus
.subscribe { (event) in
switch event {

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Missing a space before the right brace.

let image: UIImage? = visisble
? UIImage(named: "xbtn_white")?.withRenderingMode(
.alwaysOriginal
)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Wondering if breaking before .withRenderingMode could have been better.

@@ -226,7 +283,9 @@ private extension AppDelegate {
func hideHelp() -> Observable<Void> {
return Observable.create { observer in
if let presentingViewController = self.helpViewController.value?.presentingViewController {
if
let presentingViewController =

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Single let on the line after the if is rather weird.

// I couldn't figure how to swizzle this out like we do in objc.
if let _ = NSClassFromString("XCTest") { return true }
if let _ = NSClassFromString("XCTest") {
return true

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Right now I only allow single-line guards. As the guard implies a return, it doesn't need to be as visible. Not completely sure about this.

This comment has been minimized.

@orta

orta Jan 5, 2018

This makes sense to me, yes, I'm still getting used to this in prettier + JS - but it's a reasonable way to do it 👍

if let imageDicts = json["images"].object as? Array<Dictionary<String, AnyObject>> {
if
let imageDicts = json["images"].object as? Array<Dictionary<String,
AnyObject>> {

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

That's not looking nice.

let now = systemTime.date()
guard let endDate = endDate else {
// Live sales don't have end dates, and the kiosk isn't compatible with live sales.
// So we'll say any live sale is "not active"
return false
}
return (now as NSDate).earlierDate(startDate) == startDate && (now as NSDate).laterDate(endDate) == endDate
return (now as NSDate).earlierDate(startDate) == startDate && (

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Breaking into two lines on && would probably be nicer.

return "Estimate: \(dollars)"
// Try to extract non-nil low/high estimates.
// Try to extract non-nil low/high estimates.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Probably a tough one to change.

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Mmm, yeah. Not a huge deal though, could easily move the comment into the case.

@@ -34,7 +47,9 @@ class OnlineProvider<Target> where Target: Moya.TargetType {
protocol NetworkingType {
associatedtype T: TargetType, ArtsyAPIType
var provider: OnlineProvider<T> { get }
var provider: OnlineProvider<T> {

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

That one would be nicer on a single line I guess.

This comment has been minimized.

@orta

orta Jan 5, 2018

Yeah, I agree on single-lining these

// Add subviews
view.addSubview(stackView)
stackView.alignTop("0", leading: "0", bottom: nil, trailing: "0", to: view)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

One of those few classic "okay, I disagree with prettier sometimes" moments.

@orta

This comment has been minimized.

orta commented Jan 5, 2018

!!!!!!!!!

@vjeux

This is looking awesome for a first run!

@@ -16,12 +21,17 @@ class PasswordAlertViewController: UIAlertController {
exitAction.isEnabled = false
}
let cancelAction = UIAlertAction(title: "Cancel", style: .cancel) { (_) in }
let cancelAction = UIAlertAction(title: "Cancel", style: .cancel) {
_ in }

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

This looks weird

This comment has been minimized.

@orta

orta Jan 5, 2018

Yeah, this is a weird swift-ism overall, a trailing closure that does nothing.

You need to include a reference to all params, so you end up with empty functions like this. it probably does make sense to edge-case them into a single line.

@@ -11,7 +11,8 @@ class APIPingManager {
init(provider: Networking) {
self.provider = provider
letOnline = Observable<Int>.interval(syncInterval, scheduler: MainScheduler.instance)
letOnline = Observable<Int>
.interval(syncInterval, scheduler: MainScheduler.instance)

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

We have a lot of heuristics in prettier js to avoid breaking in those cases.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'm using a slightly modified version of the printMemberChain (only changed the name of the nodes).

Just to clarify: the line is rather long - would you prefer it not violate the print width or just break inside the arguments?

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Ah, my fault. Actually found the problem. Of course Observable<Int> isn't an identifier but a TypeExpr. Change the chain to always assume those to be factories.

let image: UIImage? = visisble
? UIImage(named: "xbtn_white")?.withRenderingMode(
.alwaysOriginal
)

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

In js we align the body of the ternary by two spaces so that the ) lines up with UIImage

? UIImage(
    // ...
  )

This comment has been minimized.

@orta

orta Jan 5, 2018

Yes, ^ is probably more optional

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'll make a TODO for this.

@@ -129,39 +161,53 @@ private extension AppDelegate {
// MARK: - s that do things
func () -> Observable<Void>{
func () -> Observable<Void> {

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

This function name is awesome :p

This comment has been minimized.

@ashfurrow
}
webViewController.presentingViewController?.dismiss(
animated: true
) { sendDispatchCompleted(to: observer) }

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

this felt better before.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

This will probably go away with a larger print-width. The line here is just super long already which is why the printer has to break the first line - and then it sees a chance not to break in the block.

fileprivate(set) var provider = Networking.newDefaultNetworking()
fileprivate (set) var provider = Networking.newDefaultNetworking()

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

there is an added space here, not sure what's the standard.

This comment has been minimized.

@orta

orta Jan 5, 2018

Honestly not sure, I couldn't find any examples in swift style guides. I feel like fileprivate(set) is what I'd expect, @ashfurrow?

This comment has been minimized.

@azz

azz Jan 5, 2018

I like the "space after all keywords" rule that Prettier (almost) follows.

This comment has been minimized.

@orta

orta Jan 5, 2018

Did some research, this one is interesting, because all the apple docs have no space.

However, it compiles fine both ways and I like the language consistency that's used in the issue you mentioned.

This comment has been minimized.

@natecook1000

natecook1000 Jan 5, 2018

The reason no space works better for me is that fileprivate only affects the set, not the whole declaration. That's clearer without the whitespace. (Also, this is a cool effort!)

This comment has been minimized.

@natecook1000

natecook1000 Jan 5, 2018

Thought of another way, (set) modifies fileprivate.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Yeah, set belongs to the modifier, I'm going to tweak this.

ARHockeyAppLiveID: keys.hockeyProductionSecret,
ARSegmentioWriteKey: keys.segmentWriteKey
]
)

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

Do you think that withAnalytics should be inline like it was before?

This comment has been minimized.

@orta

orta Jan 5, 2018

Treat initial named params as something worth inlining makes sense to me.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I actually have a special rule for this already, but need to modify it a bit, so that it also works if a parameter name is present.

foo(bar(
  ...
))

@vjeux I might need some guidance on what's best to express <???> in here though:

<g>ARAnalytics.setup(<???>withAnalytics:<g><line><g>withAnalytics:<line>...</g><line></g>)</g>

I want <???> to break after breaking two levels down did not work. Do I have to resort to conditionalGroup here?

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

Yeah, ConditionalGroup is needed here :( At least I couldn’t find a way without. You can look at the logic around expandLastArg for reference

segue.destination as? UINavigationController
else { return }
guard
let listingsViewController =

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

probably want to inline guard let

email: String
) -> Observable<Void> {
return Observable
.just(email)

This comment has been minimized.

@vjeux

vjeux Jan 5, 2018

In js, this would be inline (as it was before)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'm using your Chain implementation, I guess I didn't update the isFactory check correctly.

Bundle.main.object(
forInfoDictionaryKey: "CFBundleShortVersionString"
) as? String
) ?? "Unknown"

This comment has been minimized.

@orta

orta Jan 5, 2018

this kind of change exposes single line complexity well

let defaults = UserDefaults.standard
defaults.set(sale.id, forKey: "KioskAuctionID")
defaults.synchronize()
exit(1)

This comment has been minimized.

@orta

orta Jan 5, 2018

I like this

This comment has been minimized.

@ashfurrow
@@ -16,12 +21,17 @@ class PasswordAlertViewController: UIAlertController {
exitAction.isEnabled = false
}
let cancelAction = UIAlertAction(title: "Cancel", style: .cancel) { (_) in }
let cancelAction = UIAlertAction(title: "Cancel", style: .cancel) {
_ in }

This comment has been minimized.

@orta

orta Jan 5, 2018

Yeah, this is a weird swift-ism overall, a trailing closure that does nothing.

You need to include a reference to all params, so you end up with empty functions like this. it probably does make sense to edge-case them into a single line.

bottom: "-24",
trailing: "-24",
to: window
)

This comment has been minimized.

@orta

orta Jan 5, 2018

What is the max line length you're using for this? We use 120 everywhere - ( yes prettier people, we walk up-hill in all directions ;) )

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Yeah, good point. Using the default 80 right now, and was already thinking about going to 100 because of Cocoa's rather long signatures and the tabWidth of 4.

I guess running it with 120 will reduce some of the other occasions in this PR where we feel like not breaking would have been better.

This comment has been minimized.

@orta

orta Jan 5, 2018

Personally, I'd go with defaulting 120 simply because 4 spaces is convention for Swift indentation, as you have a typed language (with a very verbose legacy) with a lot of whitespace

let image: UIImage? = visisble
? UIImage(named: "xbtn_white")?.withRenderingMode(
.alwaysOriginal
)

This comment has been minimized.

@orta

orta Jan 5, 2018

Yes, ^ is probably more optional

fileprivate(set) var provider = Networking.newDefaultNetworking()
fileprivate (set) var provider = Networking.newDefaultNetworking()

This comment has been minimized.

@orta

orta Jan 5, 2018

Honestly not sure, I couldn't find any examples in swift style guides. I feel like fileprivate(set) is what I'd expect, @ashfurrow?

// I couldn't figure how to swizzle this out like we do in objc.
if let _ = NSClassFromString("XCTest") { return true }
if let _ = NSClassFromString("XCTest") {
return true

This comment has been minimized.

@orta

orta Jan 5, 2018

This makes sense to me, yes, I'm still getting used to this in prettier + JS - but it's a reasonable way to do it 👍

ARHockeyAppLiveID: keys.hockeyProductionSecret,
ARSegmentioWriteKey: keys.segmentWriteKey
]
)

This comment has been minimized.

@orta

orta Jan 5, 2018

Treat initial named params as something worth inlining makes sense to me.

guard let online = reachabilityManager?.reach else {
return stubbing
}
guard let online = reachabilityManager?.reach else { return stubbing }

This comment has been minimized.

@orta
[
"57be35d7a09a6711ab004fa5",
"57be1fb4cd530e65fe000862"
].contains(self.id) {

This comment has been minimized.

@orta

orta Jan 5, 2018

This feels a bit weird

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

👍 I should really disallow Condition Lists breaking if they only have single condition.

@azz

I've never written a line of Swift in my life but this is so awesome!

@@ -18,6 +27,6 @@ class AuctionWebViewController: WebViewController {
_ = self?.navigationController?.popViewController(animated: true)
return
}
self.present(passwordVC, animated: true) {}
self.present(passwordVC, animated: true) { }

This comment has been minimized.

@azz

azz Jan 5, 2018

In JS we'd not put a space here.

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Right now I deliberately add one to make the "do nothing" more visible. In JS you have have more around it: function(){} or () => {}.

Don't have a strong preference on this though.

@orta What's your take?

This comment has been minimized.

@orta

orta Jan 5, 2018

I also don't have a strong preference - had I not see it I'd have gone with no space, but I kinda like the space now that I see it

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

The space "feels" more "Swift" to me, but I don't know why.

@@ -262,7 +320,11 @@ private extension AppDelegate {
var fullfilmentVisible: Observable<Bool> {
return Observable.deferred {
return Observable.create { observer in
observer.onNext((self.appViewController.presentedViewController as? FulfillmentContainerViewController) != nil)
observer.onNext(
(

This comment has been minimized.

@azz

azz Jan 5, 2018

Maybe better as:

                    (self.appViewController.presentedViewController
                         as? FulfillmentContainerViewController) != nil

or

                    (self.appViewController.presentedViewController
                         as? FulfillmentContainerViewController
                    ) != nil

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

👍 I probably forgot a breakable line in front of as?.

func ==(lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {
func == (lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {

This comment has been minimized.

@azz

azz Jan 5, 2018

Maybe don't want a space here?

This comment has been minimized.

@orta

orta Jan 5, 2018

Yeah, probably want to handle the cases of ==, += etc uniquely when used as an override function 👍

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

There's a rule in SwiftLint that actually forces you to always add a trailing space when defining operators.

I guess mostly to have a single rule to avoid Brainfuck-like syntax here:

func <|(lhs: Int, rhs: Int) -> Int {}
func <|<<A>(lhs: A, rhs: A) -> A {}

Which of these would you prefer?

  1. Never add space.
  2. Always add space.
  3. Whitelist/Blacklist/Heuristic on whether to add whitespace.

This comment has been minimized.

@orta

orta Jan 5, 2018

As that rule is enabled by default in SwiftLint - I'd go with that - keep the space

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Yeah, I like it aesthetically too.

@azz

This comment has been minimized.

azz commented Jan 5, 2018

Let me know if I can help you out setting up a prettier/prettier-swift repository!

@orta

This comment has been minimized.

orta commented Jan 5, 2018

compiling Swift/LLDB/libSyntax

I guess you're using a fork for a feature, but otherwise you could link with the version in Xcode.app?

@sirlantis

Thanks so much for your sharp eyes and all your input!

@@ -11,7 +11,8 @@ class APIPingManager {
init(provider: Networking) {
self.provider = provider
letOnline = Observable<Int>.interval(syncInterval, scheduler: MainScheduler.instance)
letOnline = Observable<Int>
.interval(syncInterval, scheduler: MainScheduler.instance)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'm using a slightly modified version of the printMemberChain (only changed the name of the nodes).

Just to clarify: the line is rather long - would you prefer it not violate the print width or just break inside the arguments?

bottom: "-24",
trailing: "-24",
to: window
)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Yeah, good point. Using the default 80 right now, and was already thinking about going to 100 because of Cocoa's rather long signatures and the tabWidth of 4.

I guess running it with 120 will reduce some of the other occasions in this PR where we feel like not breaking would have been better.

let image: UIImage? = visisble
? UIImage(named: "xbtn_white")?.withRenderingMode(
.alwaysOriginal
)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'll make a TODO for this.

}
webViewController.presentingViewController?.dismiss(
animated: true
) { sendDispatchCompleted(to: observer) }

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

This will probably go away with a larger print-width. The line here is just super long already which is why the printer has to break the first line - and then it sees a chance not to break in the block.

@@ -262,7 +320,11 @@ private extension AppDelegate {
var fullfilmentVisible: Observable<Bool> {
return Observable.deferred {
return Observable.create { observer in
observer.onNext((self.appViewController.presentedViewController as? FulfillmentContainerViewController) != nil)
observer.onNext(
(

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

👍 I probably forgot a breakable line in front of as?.

fileprivate(set) var provider = Networking.newDefaultNetworking()
fileprivate (set) var provider = Networking.newDefaultNetworking()

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Yeah, set belongs to the modifier, I'm going to tweak this.

ARHockeyAppLiveID: keys.hockeyProductionSecret,
ARSegmentioWriteKey: keys.segmentWriteKey
]
)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I actually have a special rule for this already, but need to modify it a bit, so that it also works if a parameter name is present.

foo(bar(
  ...
))

@vjeux I might need some guidance on what's best to express <???> in here though:

<g>ARAnalytics.setup(<???>withAnalytics:<g><line><g>withAnalytics:<line>...</g><line></g>)</g>

I want <???> to break after breaking two levels down did not work. Do I have to resort to conditionalGroup here?

email: String
) -> Observable<Void> {
return Observable
.just(email)

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I'm using your Chain implementation, I guess I didn't update the isFactory check correctly.

[
"57be35d7a09a6711ab004fa5",
"57be1fb4cd530e65fe000862"
].contains(self.id) {

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

👍 I should really disallow Condition Lists breaking if they only have single condition.

func ==(lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {
func == (lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

There's a rule in SwiftLint that actually forces you to always add a trailing space when defining operators.

I guess mostly to have a single rule to avoid Brainfuck-like syntax here:

func <|(lhs: Int, rhs: Int) -> Int {}
func <|<<A>(lhs: A, rhs: A) -> A {}

Which of these would you prefer?

  1. Never add space.
  2. Always add space.
  3. Whitelist/Blacklist/Heuristic on whether to add whitespace.
@sirlantis

This comment has been minimized.

Owner

sirlantis commented Jan 5, 2018

@azz If you want to set up a prettier-swift repo, I'd be happy to push to it.

Compiling Swift/LLDB/libSyntax

I guess you're using a fork for a feature, but otherwise you could link with the version in Xcode.app?

@orta Thanks for challenging me on this one. I actually thought that I had to use the swift-syntax-test binary from the project but I can indeed just use swiftc -frontend -emit-syntax. Won't be an issue then! (well, you still need a built newer than the Dec 30 build that is on swift.org as there where some critical fixes in the last few days).

@azz

This comment has been minimized.

azz commented Jan 5, 2018

@sirlantis

This comment has been minimized.

Owner

sirlantis commented Jan 5, 2018

@azz Thanks = code is up! It requires a manual Swift snapshot from yesterday (sadly Swift Package Builder CI started failing on Dec 30 and no new snapshots since).

Also because libSyntax is confused by optional semicolons and parens at the moment, I sometimes need to preprocess the raw-text which requires a small hack in Prettier right now so that it is aware that I changed the raw text under its feet (see prettier/plugin-swift#1). Looking forward if you have any ideas how we could solve this properly.

@orta @azz @vjeux @natecook1000 Thanks once more for all your valuable feedback. Got a majority of the things you found smoothed out but I'm sure there will be more snapshot tests to write in the future. 🙂

@orta

This comment has been minimized.

orta commented Jan 5, 2018

Very very cool. Now this is a great reason to update our Swift versions.

textView.text = try? NSString(
contentsOf: logPath(),
encoding: String.Encoding.ascii.rawValue
) as String

This comment has been minimized.

@j-f1

j-f1 Jan 5, 2018

        textView.text = try? NSString(contentsOf: logPath(), encoding: String.Encoding.ascii.rawValue) as String
//-----------------------------------------------------------------------------|
        textView.text = try?
            NSString(
                contentsOf: logPath(),
                encoding: String.Encoding.ascii.rawValue
            ) as String

maybe?

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I haven't made up my mind on these situations yet.


Generally the following seems to be reasonable if we don't have as? or try? around and is also what Prettier JS does:

let foo = Bar(
   baz: 123
)

I just changed as? to bind stronger to the right, so I do this

fooFooFooFoo
    as? Bar

instead of

fooFooFooFoo as?
    Bar

So I'd rather keep try on the same line as the beginning of the call expr, so that would mean:

textView.text = try? NSString(
        contentsOf: logPath(),
        encoding: String.Encoding.ascii.rawValue
    ) as String

This looks a bit odd by also gives try more prominence. Might look weirder if you have a long chain though:

fooFooFooFoo = try? fooFooFooFoo.
        .fooFooFooFoo()
        .fooFooFooFoo()
@@ -40,7 +39,8 @@ class AdminPanelViewController: UIViewController {
if APIKeys.sharedKeys.stubResponses {
auctionIDLabel.text = "STUBBING API RESPONSES\nNOT CONTACTING ARTSY API"
} else {
let version = (Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String) ?? "Unknown"
let version = (Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString")
as? String) ?? "Unknown"

This comment has been minimized.

@j-f1

j-f1 Jan 5, 2018

            let version = (Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String)  ?? "Unknown"
//-----------------------------------------------------------------------------|
            let version = (
                Bundle.main.object(
                    forInfoDictionaryKey: "CFBundleShortVersionString"
                ) as? String
            ) ?? "Unknown"

?

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

I was using this version before but there were many situations in this PR where that looked odd. Prettier JS also never breaks after a grouping (, so I'm aligning with Prettier JS here.


See https://prettier.io/playground

(Bundle.main.object(fooFooFooFooFooFooFooFooFoo().fooFooFooFooFooFooFooFooFoo()) + true) * x

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Hmm, interesting. So the solution to avoid breaking on as? is to break the Bundle.main... line onto its own line?

This comment has been minimized.

@sirlantis

sirlantis Jan 6, 2018

Owner

Yes. Here's what happened before I started allowing breaking in front of as and preventing breaks in parenthesized expressions with just one element, which looked rather ugly.

Taking a closer look however, as as a much higher precedence than ?? (and != in the linked example), so the parentheses are optional. So theoretically prettier could strip the parens and print (If I'd remove the "never break on a single element in a parenthesized group):

let version = Bundle.main.object(
        forInfoDictionaryKey: "CFBundleShortVersionString"
    ) as? String ?? "Unknown"

Would need to teach the formatter about precedence and associativity though so that it could do the stripping here.

logger.log("Card Reader was Cancelled")
}
func readerGenericResponse(_ cardData: String!) {
_cardStatus.onNext("Reader received non-card data: \(cardData ?? "") ");
_cardStatus.onNext("Reader received non-card data: \(cardData ?? "") ")

This comment has been minimized.

@j-f1

j-f1 Jan 5, 2018

Is semi: true supported?

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

Right now semis are confusing the AST parser and removing them is actually necessary to get a good AST.

In the long run, while there's a split in JS community on whether semis should be used (the ASI has its quirks) I'd prefer to embrace prettier's opinionatedness and not offer the semi option for Swift at all - as the community has a clear preference for no semicolons.

This comment has been minimized.

@azz

azz Jan 5, 2018

That's something I want to encourage with plugins, just because Prettier for JS has an option for a thing that is applicable to another language, doesn't mean it should be added to that language. The options are largely about improving the adoptability of Prettier, so if semi: true won't be missed, don't add it!

This comment has been minimized.

@sirlantis

sirlantis Jan 5, 2018

Owner

@azz 👍 I actually do support trailingCommas: "all" though (I might be biased on this one).

This comment has been minimized.

@orta

orta Jan 5, 2018

Agree on no option, I've never seen Swift design guides that have semis, it's not like the JS world split. People just don't do it in Swift.

This one was probably just that we'd been working in objc-c and got used to it ;)

@ashfurrow

Okay, so first thing: thank you! I hadn't considered using prettier, but in really like your approach here.

I'd love to see the command that you used to make the changes, and I wonder how, maybe using Husky, we could automate these on a recommit hook? And I'd probably want the format checked on CI using Danger.

Looks great, thanks again!

@@ -40,7 +39,8 @@ class AdminPanelViewController: UIViewController {
if APIKeys.sharedKeys.stubResponses {
auctionIDLabel.text = "STUBBING API RESPONSES\nNOT CONTACTING ARTSY API"
} else {
let version = (Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString") as? String) ?? "Unknown"
let version = (Bundle.main.object(forInfoDictionaryKey: "CFBundleShortVersionString")
as? String) ?? "Unknown"

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Hmm, interesting. So the solution to avoid breaking on as? is to break the Bundle.main... line onto its own line?

@@ -18,6 +27,6 @@ class AuctionWebViewController: WebViewController {
_ = self?.navigationController?.popViewController(animated: true)
return
}
self.present(passwordVC, animated: true) {}
self.present(passwordVC, animated: true) { }

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

The space "feels" more "Swift" to me, but I don't know why.

let defaults = UserDefaults.standard
defaults.set(sale.id, forKey: "KioskAuctionID")
defaults.synchronize()
exit(1)

This comment has been minimized.

@ashfurrow
@@ -129,39 +161,53 @@ private extension AppDelegate {
// MARK: - s that do things
func () -> Observable<Void>{
func () -> Observable<Void> {

This comment has been minimized.

@ashfurrow
@@ -40,7 +48,8 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
defaults.removeObject(forKey: XAppToken.DefaultsKeys.TokenExpiry.rawValue)
let auctionStoryboard = UIStoryboard.auction()
let appViewController = auctionStoryboard.instantiateInitialViewController() as? AppViewController
let appViewController = auctionStoryboard.instantiateInitialViewController()
as? AppViewController

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

How it breaks this line seems really strange to me.

This comment has been minimized.

@sirlantis

sirlantis Jan 6, 2018

Owner

Yeah, the breakpoint in front of as? was rather an experiment. Would you rather have it break after as? (instead of front), after =, or after auctionStoryboard.?

This comment has been minimized.

@ashfurrow

ashfurrow Jan 6, 2018

Hmm! I think breaking after = seems the least weird 😅

return "Estimate: \(dollars)"
// Try to extract non-nil low/high estimates.
// Try to extract non-nil low/high estimates.

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Mmm, yeah. Not a huge deal though, could easily move the comment into the case.

func ==(lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {
func == (lhs: UIStoryboardSegue, rhs: SegueIdentifier) -> Bool {

This comment has been minimized.

@ashfurrow

ashfurrow Jan 5, 2018

Yeah, I like it aesthetically too.

Repository owner deleted a comment from queendrug Jan 6, 2018

Repository owner deleted a comment from queendrug Jan 6, 2018

@sirlantis

This comment has been minimized.

Owner

sirlantis commented Jan 6, 2018

@ashfurrow I forgot to mention the command that was used:

prettier --print-width 100 --write "**/*.swift"

But right now it's hard to get a Swift snapshot so it's not working out of the box. I'll let you know when it's easier to use and put up a real PR against artsy/eidolon. Then you can make the call again.

For Husky I'd recommend using the --list-different in pre-commit hooks and maybe even consider checking only the files that actually changed, as formatting a file takes roughly 100ms right now.

Also, I'd be cautious with invoking --write before everything is safe inside a commit, as I'm pretty sure there might be a bug in Swift's libSyntax or my plugin that will have prettier eat your homework.

@ashfurrow

This comment has been minimized.

ashfurrow commented Jan 8, 2018

Awesome, thanks for the follow-up! I look forward to the PR.

As an aside, I used to be really concerned that auto-formatting on commit would "eat my homework" as you put it. After working on a few codebases that do auto-format, I have yet to run into any problems that weren't caught during code review. I think it's a cultural shift, as I saw similar concerns from Artsy developers, but we've generally all come around to it 😄

Finally, and most importantly, I want to thank you for your work bringing Prettier to Swift. A Prettier Swift plugin looks like involved work and you're solving problems that haven't really been solved yet, and I just want you to know that the community is going to hugely benefit from it. Thank you.

) as String
contentsOf: logPath(),
encoding: String.Encoding.ascii.rawValue
) as String

This comment has been minimized.

@orta

orta Jan 10, 2018

This is indenting twice, which night not be what you're looking for

        textView.text = try? NSString(
                contentsOf: logPath(),
                encoding: String.Encoding.ascii.rawValue
            ) as String

This comment has been minimized.

@sirlantis

sirlantis Jan 10, 2018

Owner

Good eyes! This extra indentation is triggered by the try. It's actually something that I wanted to experiment with and apparently committed accidentally (or subconsciously?).

But it's certainly something that no one does and the extra indent is probably more confusing than helping.

ARHockeyAppBetaID: keys.hockeyBetaSecret,
ARHockeyAppLiveID: keys.hockeyProductionSecret,
ARSegmentioWriteKey: keys.segmentWriteKey
])

This comment has been minimized.

@orta
@sirlantis

This comment has been minimized.

Owner

sirlantis commented Jan 10, 2018

@ashfurrow Thank you for your kind words. You are doing an amazing job encouraging and keeping people (like me) motivated to contribute to OSS!

As an aside, I used to be really concerned that auto-formatting on commit would "eat my homework" as you put it. After working on a few codebases that do auto-format, I have yet to run into any problems that weren't caught during code review. I think it's a cultural shift, as I saw similar concerns from Artsy developers, but we've generally all come around to it.

I've experienced that fear of auto-format as well (personally and in my team). While I didn't have bad experiences with formatters for other languages so far, the formatters for Swift (SwiftLint's autofix and now Prettier Swift) are built upon very volatile AST parsers (SourceKitten and libSyntax) so I just want to underline the alpha-ness of this plugin before someone contracts Disciplinamophobia because of it.

@orta orta referenced this pull request Jan 25, 2018

Open

Better for individuals #8

@ashfurrow

This comment has been minimized.

ashfurrow commented Mar 29, 2018

Hello! I just wanted to follow up: how are things going with Prettier for Swift? Thanks again for all your work!

@orta

This comment has been minimized.

orta commented Mar 29, 2018

Now that Swift 4.1 is out, I don't think it needs a custom toolchain version of Swift. It might be more feasible to make it production worthy.

@sirlantis

This comment has been minimized.

Owner

sirlantis commented Mar 29, 2018

@ashfurrow

This comment has been minimized.

ashfurrow commented Mar 29, 2018

Cool! Really excited to see this come to fruition!

@sirlantis

This comment has been minimized.

Owner

sirlantis commented Mar 31, 2018

Quick update - sadly 4.1 has a bug which swallows some newlines, so I'm stuck with 4.2 snapshots for now (🤞 for 4.2 being a better candidate).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment