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

Honor .sortedKeys #11

Closed
wants to merge 11 commits into from
Closed

Honor .sortedKeys #11

wants to merge 11 commits into from

Conversation

qmoya
Copy link
Contributor

@qmoya qmoya commented Dec 2, 2018

I realized that the .sortedKeys formatting option was being ignored. Sorting keys is a convenient thing to have, particularly when testing—else, comparing against strings breaks all the time.

This PR wants to fix that.

It also adds a .swiftformat file. Its contents were inferred automatically by SwiftFormat itself. I just wanted my contribution to be consistent with the project’s de facto style, but I think all projects benefit from explicit formatting rules—thus this addition.

@qmoya qmoya changed the title Honor .sortedKeys Honor .sortedKeys Dec 2, 2018
@codecov
Copy link

codecov bot commented Dec 2, 2018

Codecov Report

Merging #11 into master will increase coverage by 1.72%.
The diff coverage is 25.97%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #11      +/-   ##
=========================================
+ Coverage   24.58%   26.3%   +1.72%     
=========================================
  Files          12      12              
  Lines        1554    1589      +35     
=========================================
+ Hits          382     418      +36     
+ Misses       1172    1171       -1
Impacted Files Coverage Δ
...rces/XMLCoder/Decoder/DecodingErrorExtension.swift 0% <ø> (ø) ⬆️
Sources/XMLCoder/ISO8601DateFormatter.swift 0% <ø> (ø) ⬆️
...urces/XMLCoder/Encoder/XMLReferencingEncoder.swift 0% <0%> (ø) ⬆️
...rces/XMLCoder/Encoder/EncodingErrorExtension.swift 0% <0%> (ø) ⬆️
...s/XMLCoder/Decoder/XMLKeyedDecodingContainer.swift 12.29% <10.6%> (+0.4%) ⬆️
Sources/XMLCoder/Encoder/XMLEncoder.swift 22.93% <20.11%> (+0.04%) ⬆️
Sources/XMLCoder/Decoder/XMLDecoder.swift 22.61% <21.66%> (ø) ⬆️
Sources/XMLCoder/XMLKey.swift 25% <50%> (ø) ⬆️
...XMLCoder/Decoder/XMLUnkeyedDecodingContainer.swift 7.56% <7.54%> (ø) ⬆️
Sources/XMLCoder/Encoder/XMLEncodingStorage.swift 87.5% <83.33%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f65d765...f35bb7f. Read the comment docs.

Copy link
Collaborator

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution! I'd be happy to merge this after we get some consistency in trailing whitespaces on empty newlines, spaces around range operators and CI failures are fixed. I'd see no problem in merging this PR after that, but in the future it would be great if unrelated changes are split into multiple PRs, in this case one PR could be a fix for .sortedKeys and another one only related to SwiftFormat.

@@ -23,7 +23,7 @@ internal extension DecodingError {
let description = "Expected to decode \(expectation) but found \(_typeDescription(of: reality)) instead."
return .typeMismatch(expectation, Context(codingPath: path, debugDescription: description))
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like trailing whitespaces on a blank line, was this committed by mistake or added by SwiftFormat? In either case it would be great to avoid these

let trailingUnderscoreRange = stringKey.index(after: lastNonUnderscore)..<stringKey.endIndex
let keyRange = firstNonUnderscore ... lastNonUnderscore
let leadingUnderscoreRange = stringKey.startIndex ..< firstNonUnderscore
let trailingUnderscoreRange = stringKey.index(after: lastNonUnderscore) ..< stringKey.endIndex
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding spaces around range operators is an interesting style I haven't seen before, including this library. In general I'd prefer to stick to having no spaces around those, which is the same as what the Swift standard library does for example. Does SwiftFormat support that?

let result: Item = try! decode(xml)

// then
let expected = Item(id: "1542637462", creator: "Francisco Moya", children: nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused why children gets a nil value here, isn't it supposed to be Attribute(name: "Xpos", value: "0")?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Max! Thanks—by mistake, I pushed some things to this PR that should have gone elsewhere. My last intended commit was ‘Complete if #availables’. I’ll revert as appropriate and fix the automated formatting. Thanks!

@qmoya qmoya mentioned this pull request Dec 3, 2018
@qmoya qmoya closed this Dec 3, 2018
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 15, 2019
bwetherfield added a commit to bwetherfield/XMLCoder that referenced this pull request Jul 26, 2019
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 26, 2019
Add benchmark baselines

Pull in tests (CoreOffice#11)

Add Decoding support for choice elements (CoreOffice#15)

Fix indentation (CoreOffice#16)

Replace usage of XCTUnrwap (CoreOffice#19)

Add nested choice tests (CoreOffice#18)

Add falsely passing double array roundtrip test (CoreOffice#17)
bwetherfield pushed a commit to bwetherfield/XMLCoder that referenced this pull request Jul 27, 2019
Add benchmark baselines

Pull in tests (CoreOffice#11)

Add Decoding support for choice elements (CoreOffice#15)

Fix indentation (CoreOffice#16)

Replace usage of XCTUnrwap (CoreOffice#19)

Add nested choice tests (CoreOffice#18)

Add falsely passing double array roundtrip test (CoreOffice#17)
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.

2 participants