Skip to content
This repository has been archived by the owner on Jan 29, 2022. It is now read-only.

Suggested changes #3

Merged
merged 3 commits into from
Jun 14, 2018
Merged

Suggested changes #3

merged 3 commits into from
Jun 14, 2018

Conversation

kmahar
Copy link
Contributor

@kmahar kmahar commented Jun 13, 2018

  • 2->4 spaces since we're using 4 in the driver and the sample app
  • use the new MongoSwift initialize and cleanup functions
  • make error messages actually print out
  • helper for getting status explanation
  • use mongo embedded constants directly
  • get rid of force unwraps
  • import Foundation for sake of clarity (it won't build in TestMongoMobile if we explicitly import MongoSwift though)

the sample app builds and seems to work ok with these changes.

@kmahar kmahar requested a review from mbroadst June 13, 2018 21:56
@@ -1,124 +1,137 @@
import Foundation
import mongo_embedded
Copy link
Contributor

Choose a reason for hiding this comment

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

[q] Should this also have import MongoSwift?

Copy link
Contributor Author

@kmahar kmahar Jun 13, 2018

Choose a reason for hiding this comment

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

I tried, but MongoMobile won't build within another project (such as TestMongoMobile) if it's there and says it can't find the package. I guess with the way MongoSwift ends up getting included, the code is available but not under that name

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, since MongoSwift is being vendored in it no longer exists in that package name. This is an unfortunate sideeffect of not being able to properly set up the dependency between the libraries.

@mbroadst mbroadst merged commit 6ca646f into master Jun 14, 2018
@mbroadst mbroadst deleted the changes branch June 14, 2018 18:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants