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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

test formatting issues as part of sanity check #84

Merged
merged 2 commits into from
Jul 30, 2019

Conversation

tomerd
Copy link
Member

@tomerd tomerd commented Jul 30, 2019

motivaiton: unified format

changes:

  • use official docker image 馃帀
  • fix outstanding formatting issues
  • add a call to swiftformat as part of sanity script
  • fix sanity script language check debugging statements

@tomerd tomerd requested review from ktoso and weissi and removed request for ktoso July 30, 2019 02:07
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :) One question but not an issue really

let systemStderr = Glibc.stderr!
let systemStdout = Glibc.stdout!
let systemStderr = Glibc.stderr!
let systemStdout = Glibc.stdout!
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit / question: I was under the impression "official formatting" for #if is to not indent?

Either way, happy with having an automatted formatted so whichever we want is fine, just confused about this one.

Copy link
Member Author

@tomerd tomerd Jul 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally i don't have a strong opinion on concrete format rules. what we agreed on in the past is captured in the repo's .swiftformat file, so we can change it if we want to agree on a different set of rules

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah same, though this one is surprising... curious if choice was conscious on the #ifs @weissi ? As mentioned, not an issue and LGTM as I'm more than happy to have automated formatter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go with whatever Xcode does just because it's a pain otherwise because it'll re-indent in the way it thinks it's right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
set {
metadata[metadataKey] = newValue
self.metadata[metadataKey] = newValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

@tomerd
Copy link
Member Author

tomerd commented Jul 30, 2019

/cc @ianpartridge

@@ -1,56 +1,35 @@
ARG ubuntu_version=18.04
FROM ubuntu:$ubuntu_version
ARG swift_version=5.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃檶

motivaiton: unified format

changes:
* use official docker image 馃帀
* fix outstanding formatting issues
* add a call to swiftformat as part of sanity script
* fix sanity script language check debugging statements
@tomerd tomerd merged commit 2637350 into apple:master Jul 30, 2019
@tomerd tomerd added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants