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 NSCopying support to FEMMapping #74

Merged
merged 4 commits into from
Sep 27, 2016

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Sep 27, 2016

Also change a few FEMMapping readonly properties to readwrite

Discussed here: #73

@dimazen dimazen changed the base branch from develop to master September 27, 2016 08:34
@dimazen
Copy link
Contributor

dimazen commented Sep 27, 2016

I have changed base branch to master, because develop is no longer in use.

@dimazen
Copy link
Contributor

dimazen commented Sep 27, 2016

It also looks like a shallow copy and may cause unexpected behavior when Mapping A gets copied and later attributes updates somehow. Would you be mind to add copying support to Attribute and Relationship as well? Sorry for not mentioning it before.

@k06a k06a force-pushed the feature/FEMMapping-support-NSCopying branch from 088aa37 to 8dbf482 Compare September 27, 2016 09:34
@codecov-io
Copy link

codecov-io commented Sep 27, 2016

Current coverage is 92.67% (diff: 0.00%)

Merging #74 into master will decrease coverage by 0.93%

@@             master        #74   diff @@
==========================================
  Files            27         27          
  Lines          3068       3099    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           2872       2872          
- Misses          196        227    +31   
  Partials          0          0          

Powered by Codecov. Last update ecb6e66...667c24f

Copy link
Contributor

@dimazen dimazen left a comment

Choose a reason for hiding this comment

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

After two minor fixes look good to merge.

#pragma mark - NSCopying

- (instancetype)copyWithZone:(NSZone *)zone {
FEMMapping *mapping = [[self.class allocWithZone:zone] initWithEntityName:self.entityName];
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to use

if (self.entityName != nil) { 
     = [self.class ....] initWithEntityName:];
else { 
     = [self.class ....] initWithObjectClass:];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also assignment of objectClass below needs to be removed in favor of using initializers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with conditional initialiser call


- (instancetype)copyWithZone:(NSZone *)zone {
FEMMapping *mapping = [[self.class allocWithZone:zone] initWithEntityName:self.entityName];
mapping.objectClass = self.objectClass;
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to override setters and nil-out counterpart. Otherwise you can set both objectClass and entityName which might look strange

- (void)setEntityName:(NSString *)entityName 
{
    _objectClass = nil;
    _entityName = [entityName copy];
}

- (void)setObjectClass:(Class)objetClass
{
    _objectClass = objetClass;
    _entityName = nil;
}

Copy link
Contributor Author

@k06a k06a Sep 27, 2016

Choose a reason for hiding this comment

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

Done, and improved for case setting nil to already nilled property.

@dimazen dimazen merged commit 952deab into Yalantis:master Sep 27, 2016
@dimazen
Copy link
Contributor

dimazen commented Sep 27, 2016

I will rollout 1.1.1 tomorrow with added documentation from #69

@k06a
Copy link
Contributor Author

k06a commented Sep 27, 2016

@dimazen Thanks!

@k06a k06a deleted the feature/FEMMapping-support-NSCopying branch September 27, 2016 10:58
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