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

CoreData: Change fetchRequest to makeFetchRequest #726

Merged
merged 10 commits into from
Jul 6, 2020
Merged

CoreData: Change fetchRequest to makeFetchRequest #726

merged 10 commits into from
Jul 6, 2020

Conversation

davidrothera
Copy link
Contributor

Fixes #725

Technically I can see this as a breaking change so added as such in the CHANGELOG

@SwiftGen-Eve
Copy link

SwiftGen-Eve commented Jun 23, 2020

Hey 👋 I'm Eve, the friendly bot watching over SwiftGen 🤖

Thanks a lot for your contribution!


Seems like everything is in order 👍 You did a good job here! 🤝

Generated by 🚫 Danger

@djbe djbe added this to the 7.0.0 milestone Jun 23, 2020
Copy link
Member

@djbe djbe left a comment

Choose a reason for hiding this comment

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

You'll need to apply the same change to the swift 4 template.

I'm not 100% certain what the naming conventions are, and am very bad at it myself, so unless there's a better suggestion, let's go with createFetchRequest().

CHANGELOG.md Outdated Show resolved Hide resolved
@davidrothera
Copy link
Contributor Author

You'll need to apply the same change to the swift 4 template.

I'm not 100% certain what the naming conventions are, and am very bad at it myself, so unless there's a better suggestion, let's go with createFetchRequest().

Yeah I'm not sure on whether this is the correct name for the method either, it was either this or makeFetchRequest when I was thinking about it.

@djbe
Copy link
Member

djbe commented Jun 23, 2020

@davidrothera thx for the quick PR!

Maybe we can avoid this being a breaking change, if we keep the old method name, and add a new method createFetchRequest(). We could maybe even add a @available annotation marking the old one as deprecated:

@available(*, deprecated, renamed: "createFetchRequest", message: "To avoid collisions with the less concrete method in `NSManagedObject`, please use `createFetchRequest()` instead.")
@nonobjc internal class func fetchRequest() -> NSFetchRequest<SecondaryEntity> {
  return NSFetchRequest<SecondaryEntity>(entityName: entityName)
}

@nonobjc internal class func createFetchRequest() -> NSFetchRequest< SecondaryEntity > {
  return NSFetchRequest<SecondaryEntity>(entityName: entityName)
}

@davidrothera
Copy link
Contributor Author

@davidrothera thx for the quick PR!

Maybe we can avoid this being a breaking change, if we keep the old method name, and add a new method createFetchRequest(). We could maybe even add a @available annotation marking the old one as deprecated:

@available(*, deprecated, renamed: "createFetchRequest", message: "To avoid collisions with the less concrete method in `NSManagedObject`, please use `createFetchRequest()` instead.")
@nonobjc internal class func fetchRequest() -> NSFetchRequest<SecondaryEntity> {
  return NSFetchRequest<SecondaryEntity>(entityName: entityName)
}

@nonobjc internal class func createFetchRequest() -> NSFetchRequest< SecondaryEntity > {
  return NSFetchRequest<SecondaryEntity>(entityName: entityName)
}

Thanks for the suggestion, yeah I would always prefer not having to break backwards compatibility. I'll get this changed shortly 👍

@davidrothera
Copy link
Contributor Author

I've added the method back in along with a deprecation notice, I updated the CHANGELOG to move it to the "New Features" section but not sure whether it would be best there or under one of the other headings.

@davidrothera davidrothera requested a review from djbe June 23, 2020 23:46
Copy link
Member

@djbe djbe left a comment

Choose a reason for hiding this comment

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

So we also need to update:

  • swift 4 docs
  • readme usage example code
  • playgrounds page code

Documentation/templates/coredata/swift5.md Show resolved Hide resolved
@djbe
Copy link
Member

djbe commented Jun 23, 2020

Changelog location is ok for now, we can always move it later if really needed.

@djbe djbe modified the milestones: 7.0.0, 6.3.0 Jun 23, 2020
@davidrothera davidrothera requested a review from djbe June 24, 2020 07:50
@davidrothera
Copy link
Contributor Author

Hey @djbe, have you had chance to look over the latest changes you requested?

@djbe
Copy link
Member

djbe commented Jul 4, 2020

Allright, seems like those last changes cover everything.

One unfortunate bit is that I read through the API design guidelines:

Begin names of factory methods with “make”, e.g. x.makeIterator().

So it should become makeFetchRequest() 😅 Luckily, we can find/replace now that the initial effort is done.
Feel free to rebase your commits if you find that easier.

@davidrothera
Copy link
Contributor Author

Good find and nice to know, there are probably some places in my own code which I should clean up in that case as well 😝

@davidrothera davidrothera changed the title CoreData: Change fetchRequest to createFetchRequest CoreData: Change fetchRequest to makeFetchRequest Jul 4, 2020
@davidrothera
Copy link
Contributor Author

Ok should be good to go now @djbe

Copy link
Member

@djbe djbe left a comment

Choose a reason for hiding this comment

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

Nice, thx for the update!

I'm approving this now, but not merging it yet, as we still need to release 6.2.1. Depending on how that release goes, we may need a 6.2.2. After all that, this can be merged for 6.3.0.

@djbe djbe merged commit c55b20f into SwiftGen:develop Jul 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants