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

Add parameter to ignore entities #378

Merged
merged 6 commits into from
Oct 1, 2018
Merged

Conversation

q231950
Copy link
Contributor

@q231950 q231950 commented Sep 25, 2018

Hi @rentzsch

Recently I have been looking into using the Xcode CoreData generated NSManagedObject subclasses. The code base we are working on is kind of big and we have plenty of Mogenerator generated classes.

In order to slowly migrate to the Xcode generated classes we need a way to use both generators at the same time.

Xcode already supports this by allowing to enable/disable code generation per entity. This pull request adds support for ignoring entities when generating with Mogenerator (I looked a bit but couldn't find any other solution to ignore entities).

Tests passed with MacOSX10.14.sdk and I added another test for checking if the parameter works.

Thank you and I am looking forward to your feedback!

@rentzsch
Copy link
Owner

Hey Martin,

I like the exclusion idea for better Xcode interop and your code.

However, I think it would make more sense to opt out of mogenerator's codegen at the entity's userInfo level instead of passing in a new argument.

I'm thinking something like mogenerator.skipgen to match the mogenerator.readonly and mogenerator.customBaseClass naming scheme.

What do you think?

@rentzsch
Copy link
Owner

Actually, I like your initial naming, so how about mogenerator.ignore?

Doesn't matter the value of the key, just the key's presence would be enough for mogenerator to ignore it.

@atomicbird
Copy link
Collaborator

atomicbird commented Sep 26, 2018

I agree. This is a per-entity setting, and mogenerator already does per-entity checks. It fits with the mogenerator approach to configure this in the model editor instead of at the command line.

@q231950
Copy link
Contributor Author

q231950 commented Sep 26, 2018

Hey @rentzsch @atomicbird, in the initial implementation I actually started out by having it in the entities. I will update the pull request and give it another try.

Thanks for the feedback 🙂

@q231950
Copy link
Contributor Author

q231950 commented Sep 26, 2018

I have added it to the entity now, it looks much better like this. I moved isIgnored into a new category: @interface NSEntityDescription (userInfo). Please let me know if you're fine with that.

@rentzsch
Copy link
Owner

rentzsch commented Sep 27, 2018

Overall looks great.

I see two outstanding issues:

  1. Most important: @atomicbird has been putting in a ton of work into the next release, and I'm fine with merging this current PR but want to clear with him that now's a good time to do it.

  2. I don't see a reason now there's two separate :objc tasks in test's Rakefile. Either I can just merge the includes_uncle lines into the first :objc task or you can do it. Unless I'm missing something.

@q231950
Copy link
Contributor Author

q231950 commented Sep 27, 2018

  1. No worries for getting this into this or another release. I'd be very happy when it gets in at all since I see that it really helps us and others might also benefit from it.

  2. The idea was to have a specific test to assert that reading the entities' user info works. What do you think about having a separate method to have this assertion for both, the Swift and the Objective-C build?

def assert_entity_user_info_respected
  # mogenerator.ignore
  includes_uncle = Dir.entries('./MOs').any? { |file| file.include?('Uncle') }
  raise 'Failed: Ignored entities should not be generated' if includes_uncle
end

desc 'Generate, Compile and Run Objective-C'
task :objc do
  Rake::Task[:clean].execute
  gen_and_compile_objc(MOGENERATOR_PATH, '', '')
  assert_entity_user_info_respected
  Rake::Task[:clean].execute
end

desc 'Generate, Compile and Run Swift'
task :swift do
  Rake::Task[:clean].execute
  gen_and_compile_swift(MOGENERATOR_PATH, '')
  assert_entity_user_info_respected
  Rake::Task[:clean].execute
end

@rentzsch
Copy link
Owner

  1. Cool. We'll wait on @atomicbird's feedback.

  2. That looks great, please put that into your Pull Request.

@q231950
Copy link
Contributor Author

q231950 commented Sep 28, 2018

@rentzsch, @atomicbird I updated all the things, looking forward to it, thank you 🙂

@atomicbird
Copy link
Collaborator

I haven't had a chance to review this in detail but if it looks good to you @rentzsch then by all means merge it. Most of my recent work has gone into the swift42 branch, and I'll deal with any conflicts that arise there.

@rentzsch rentzsch merged commit 48fc1ff into rentzsch:master Oct 1, 2018
@q231950
Copy link
Contributor Author

q231950 commented Oct 2, 2018

Awesome @rentzsch @atomicbird, thanks for merging 🎉

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.

3 participants