-
Notifications
You must be signed in to change notification settings - Fork 113
Update swiftformat #196
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
Update swiftformat #196
Conversation
@swift-server-bot test this please |
2cb43b3
to
7a8a70f
Compare
@@ -145,16 +146,18 @@ struct ExchangeRatesCalculator { | |||
dateFormatter.dateFormat = "dd/MMM/yy" | |||
let interval: DateInterval? | |||
if let period = try document.nodes(forXPath: "/exchangeRateMonthList/@Period").first?.stringValue, | |||
period.count == 26 { | |||
period.count == 26 | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit weird?
let rate = Decimal(string: rateString, locale: self.locale) { | ||
// We must parse the decimal data using the UK locale, not the system one. | ||
let rate = Decimal(string: rateString, locale: self.locale) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah these are pretty weird... why does it cut down the {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this is odd, is there an option to turn this off?
public extension AppSync { | ||
enum Response<ResultType: Encodable>: Encodable { | ||
extension AppSync { | ||
public enum Response<ResultType: Encodable>: Encodable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are good actually 👍
@@ -15,7 +15,7 @@ | |||
import struct Foundation.Date | |||
|
|||
// https://docs.aws.amazon.com/lambda/latest/dg/with-ddb.html | |||
public struct DynamoDB { | |||
public enum DynamoDB { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is honestly crazy... a formatter changing types just because it feels like it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea this is nasty imo - is there an option to turn this off?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this pretty clever... It detect's that this is used as a namespace and makes it explicit:
enumNamespaces
-> Converts types used for hosting only static members into enums.
https://github.com/nicklockwood/SwiftFormat/blob/master/Rules.md#enumnamespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's NOT up to a formatting tool to decide how I'm designing and using types. I.e. it's a struct because it will gain fields in the future etc.
We really should not allow this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, its clever, but it should a human decision to change a type not a formatting tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disabled the matching swiftformat
rule. Since this change is actually clever in this case, do we want to keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing an enum
to a struct
is also API breaking...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weissi we go the other way though. struct -> enum. just out of curiosity: in which way would enum -> struct be source breaking, if it is just about a namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import enum Foo.Bar
will break. No tool should ever switch enum
to or from struct
(or class or something). The programmer needs to decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly fine, a few changes are a bit worrying
7a8a70f
to
283202b
Compare
--extensionacl on-declarations | ||
--disable typeSugar | ||
--disable andOperator | ||
--disable wrapMultilineStatementBraces | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please disable the enum namespaces
Ping @fabianfett |
- Update swiftformat from 0.44 to 0.47 - Add format option `--extensionacl on-declarations` to ensure ACL is never set on extensions - Add format option `--disable typeSugar` to ensure `&&` is not replaced with `,`
283202b
to
cb2ea4d
Compare
Thanks! That sounds good to me 👍 |
Motivation:
Our
swiftformat
should be more or less up to date. We should not require contributors to get a special version.Modifications:
5.2
(Since this is the swift version we require anyway)--extensionacl on-declarations
to ensure ACL is never set on extensions--disable typeSugar
to ensure&&
is not replaced with,
Result:
swift-aws-lambda-runtime