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

Minor reorganizations in YapDatabaseExtensions.swift #78

Merged
merged 3 commits into from Jan 27, 2016

Conversation

cdzombak
Copy link
Contributor

@cdzombak cdzombak commented Jan 7, 2016

These changes are minor, opinion-based reorganizations with no functional change; thus this PR may be closed with no hard feelings if this code churn is undesirable.

  • d21165b separates YapDB.Index's Hashable and CustomStringConvertible implementations; they're not really related and it took me a moment to realize there wasn't a reason for them to be grouped together like that.

  • 8df385e groups free functions operating on Persistables alongside the Persistable protocol definition, as so:

This minor reorganization just makes reading the implementation a little clearer, IMO. This is not a functional change, so rejection of this change would be understandable.
This results in the following structure in Xcode, wherein free functions operating on Persistable are grouped with the Persistable definition:

![](https://dropbox.dzombak.com/ghimages/Persistable.png)

This minor reorganization just makes reading the implementation a little clearer, IMO. This is not a functional change, so rejection of this change would be understandable.
@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 7, 2016

iOS and OS X tests seem to be passing locally for me.

@danthorpe
Copy link
Owner

Yeah, it's probably just my little Mac Mini...

@danthorpe
Copy link
Owner

Can you make sure that you've pulled the latest development into your branches? I updated the test scripts to use iOS 9.2 yesterday.

@danthorpe
Copy link
Owner

That's why the iOS tests are failing - it's timing out looking for the simulator.

@danthorpe
Copy link
Owner

But the changes look good, I don't mind the churn if overall readability increases 👍 Thanks!

@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 7, 2016

I believe I've pulled in the latest development via cdzombak@61564fa .

@danthorpe
Copy link
Owner

Yeah, I think it's okay now, last one is just testing on iOS. There were some issues with Carthage on the build machines, seems to have resolved itself now.

@codecov-io
Copy link

Current coverage is 83.73%

Merging #78 into development will not affect coverage as of 61564fa

@@            development     #78   diff @@
===========================================
  Files                27      27       
  Stmts               965     965       
  Branches              0       0       
  Methods                               
===========================================
  Hit                 808     808       
  Partial               0       0       
  Missed              157     157       

Review entire Coverage Diff as of 61564fa

Powered by Codecov. Updated on successful CI builds.

@danthorpe
Copy link
Owner

Will leave these here for a bit in case there are any more things you spot.

@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 7, 2016

Will leave these here for a bit in case there are any more things you spot.

👍 thanks!

@cdzombak
Copy link
Contributor Author

cdzombak commented Jan 7, 2016

Will leave these here for a bit in case there are any more things you spot.

👍 thanks!

@danthorpe I probably won't have any more proposals in the very near term. Thanks for looking over my changes!

@danthorpe
Copy link
Owner

@cdzombak okay - great stuff, will merge your PRs, thanks!

danthorpe added a commit that referenced this pull request Jan 27, 2016
Minor reorganizations in YapDatabaseExtensions.swift
@danthorpe danthorpe merged commit 3fd6b52 into danthorpe:development Jan 27, 2016
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.

None yet

3 participants