Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update FetchRequest #86

Closed
wants to merge 3 commits into from
Closed

Conversation

MaxHasADHD
Copy link

@MaxHasADHD MaxHasADHD commented Jun 5, 2016

Pull request checklist

This fixes issue #___.

What's in this pull request?

Uses a cleaner FetchRequest.

Maximilian Litteral and others added 3 commits June 5, 2016 15:12
@codecov-io
Copy link

codecov-io commented Jun 5, 2016

Current coverage is 90.11%

Merging #86 into develop will increase coverage by 0.02%

@@            develop        #86   diff @@
==========================================
  Files            13         13          
  Lines           434        435     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            391        392     +1   
  Misses           43         43          
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 5413b36...1bf6f9e

@jessesquires
Copy link
Owner

Thanks @MaxHasADHD ! 😄

This isn't quite the direction that I think we should take the library.

  • I think FetchRequest should continue to be initialized with an entity description. Initializing a fetch request with a context is really awkward considering we have context.fetch(request: r)
  • I would like to see improvements around initializing an entity description. Using "\(self.self)" is close, but not quite there. I'd like to use the approaches that Daniel outlines here: https://realm.io/news/tryswift-daniel-eggert-modern-core-data/

I've opened #87 to track this. 😊

@MaxHasADHD
Copy link
Author

I'm watching the talk right now! I'm currently reading his book as well. Just wondering though, would "\(self.self)" ever be wrong? Or just maybe in the future it could change to not be the entities name like I expect?

@jessesquires
Copy link
Owner

Or just maybe in the future it could change to not be the entities name like I expect?

Exactly.

With Daniel's approach, we can have the classes provide an entityName.

Then, we might be able to provide a default implementation that returns "\(self.self)" (or similar). This way, we could provide good defaults, but allow clients to override. 😄

@MaxHasADHD
Copy link
Author

I like that idea! I'm going to implement this into my project :) Thanks

@jessesquires
Copy link
Owner

We'll provide more of these kinds of things in this library soon 😊

@saurabytes
Copy link
Collaborator

safer to use "\(self.dynamicType.self)" also get rids of the module name and will leave you with the class name which is in most case is the entity name

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants