Skip to content

SWIFT-858: Duplicate pure Swift BSON breaking changes in driver #477

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
May 28, 2020
Merged

SWIFT-858: Duplicate pure Swift BSON breaking changes in driver #477

merged 11 commits into from
May 28, 2020

Conversation

@nbbeeken
Copy link
Contributor

@nbbeeken nbbeeken commented May 20, 2020

All changes listed in ticket:
https://jira.mongodb.org/browse/SWIFT-858

Additionally changed:

  • BSONType is UInt8 instead of UInt32

@nbbeeken nbbeeken requested review from kmahar and patrickfreed May 20, 2020 18:49
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020 •
edited
Loading

Codecov Report

Merging #477 into master will decrease coverage by 0.13%.
The diff coverage is 94.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #477      +/-   ##
==========================================
- Coverage   76.89%   76.75%   -0.14%     
==========================================
  Files         125      125              
  Lines       13012    13545     +533     
==========================================
+ Hits        10005    10396     +391     
- Misses       3007     3149     +142
Impacted Files Coverage Δ
Sources/MongoSwift/BSON/Overwritable.swift 100.00% <ø> (ø)
Sources/MongoSwift/BSON/BSONDocument.swift 88.36% <50.00%> (ø)
Sources/MongoSwift/BSON/BSONDecoder.swift 67.21% <66.66%> (+1.03%) ⬆️
Sources/MongoSwift/BSON/BSONValue.swift 75.82% <85.71%> (-0.19%) ⬇️
Sources/MongoSwift/BSON/BSON.swift 80.59% <100.00%> (ø)
Sources/MongoSwift/BSON/BSONDocumentIterator.swift 89.53% <100.00%> (ø)
Tests/BSONTests/BSONCorpusTests.swift 87.50% <100.00%> (ø)
Tests/BSONTests/BSONValueTests.swift 98.98% <100.00%> (+0.14%) ⬆️
Tests/BSONTests/CodecTests.swift 99.84% <100.00%> (ø)
Tests/BSONTests/Document+SequenceTests.swift 99.57% <100.00%> (ø)
... and 15 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 78f1fd3...a296138. Read the comment docs.

kmahar
kmahar suggested changes May 21, 2020
View reviewed changes
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

looks good! few small things to fix.

@nbbeeken nbbeeken force-pushed the SWIFT-858/break-bson branch from 9d0b2b2 to 835cb46 Compare May 21, 2020 15:11
kmahar
kmahar reviewed May 21, 2020
View reviewed changes
@kmahar kmahar mentioned this pull request May 24, 2020
Merged
kmahar
kmahar reviewed May 26, 2020
View reviewed changes
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

one more thing; also I think we need a rebase to resolve merge conflicts, and it looks like the tests need an update in order to compile

Sources/MongoSwift/BSON/BSONValue.swift Outdated
public var rawValue: UInt8

public init(_ value: UInt8) { self.rawValue = value }
public init?(rawValue: UInt8) { self.rawValue = rawValue }
Copy link
Contributor

@kmahar kmahar May 26, 2020

Choose a reason for hiding this comment

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

sorry for the back and forth but I think maybe we should omit RawRepresentable conformance because the public initializers here allows users to bypass the validation of their custom subtype value. This is not great because a user could start using eg 0x07 as a custom subtype and then we could add a conflicting subtype in a future server version. 0x07-0x7F are reserved for future non-custom subtypes.

Copy link
Contributor Author

@nbbeeken nbbeeken May 26, 2020

Choose a reason for hiding this comment

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

Yea I think it would make more sense to not conform, I'll revert

Copy link
Contributor

@patrickfreed patrickfreed May 27, 2020

Choose a reason for hiding this comment

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

Sorry to reignite this conversation, but could we not just return nil if a user specified a reserved but unused value? From the docs of RawRepresentable, this seems like the ideal case to conform to it (i.e. modeling a C style enum).

Copy link
Contributor

@kmahar kmahar May 27, 2020

Choose a reason for hiding this comment

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

that is a good point, didn't think of that lol. i'm fine with returning nil for reserved subtypes

@nbbeeken nbbeeken force-pushed the SWIFT-858/break-bson branch from 3b0878b to 34f3df1 Compare May 26, 2020 20:32
kmahar
kmahar reviewed May 27, 2020
View reviewed changes
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

ok 1 more comment again :)
also sorry for introducing more merge conflicts.... again we totally should have merged this before i did all the prefixing 🙈

@nbbeeken nbbeeken force-pushed the SWIFT-858/break-bson branch from f09201d to 52653b0 Compare May 27, 2020 16:36
kmahar
kmahar reviewed May 27, 2020
View reviewed changes
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

new stuff LGTM but there are some issues from rebasing

kmahar
kmahar approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@kmahar kmahar left a comment

Choose a reason for hiding this comment

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

LGTM assuming travis and evergreen pass!

patrickfreed
patrickfreed reviewed May 27, 2020
View reviewed changes
patrickfreed
patrickfreed reviewed May 28, 2020
View reviewed changes
patrickfreed
patrickfreed reviewed May 28, 2020
View reviewed changes
@nbbeeken nbbeeken merged commit ba1825d into master May 28, 2020
@nbbeeken nbbeeken deleted the SWIFT-858/break-bson branch May 28, 2020 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@kmahar kmahar kmahar approved these changes

@patrickfreed patrickfreed patrickfreed approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@nbbeeken @codecov-commenter @patrickfreed @kmahar