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

Nullify unresolved links (optional) #58

Merged
merged 1 commit into from
Jan 5, 2015
Merged

Nullify unresolved links (optional) #58

merged 1 commit into from
Jan 5, 2015

Conversation

tomxor
Copy link
Contributor

@tomxor tomxor commented Jan 5, 2015

No description provided.

@grncdr
Copy link

grncdr commented Jan 5, 2015

Would it maybe be better to have an UnresolvedLink extends CDAResource type? null seems a little harsh.

edit: by "better" I mean it could allow clients to react to unresolved links with a bit more information.

@tomxor
Copy link
Contributor Author

tomxor commented Jan 5, 2015

@grncdr That introduces the same problem, only instead of a Map you'd get a UnresolvedLink. Say you have an entry linking to another one, you would be forced to do something like:

CDAResource linkedResource = myEntry.getFields().get("linked");
if (linkedResource instanceof UnresolvedLink) { 
  // ..
} else {
  // ..
}

That unfortunately forces the caller to use reflection in order to figure that out :(

@grncdr
Copy link

grncdr commented Jan 5, 2015

That's a fair point. Is there some way that we could return null and still let the caller figure out where the broken link is pointing?

@tomxor
Copy link
Contributor Author

tomxor commented Jan 5, 2015

I don't think so, AFAIK the way that the iOS SDK deals with this is simply create a blank CDAEntry/CDAAsset, which potentially could lead to another problem, as one would expect non-null values for required fields and in such cases those would be null.

@tomxor
Copy link
Contributor Author

tomxor commented Jan 5, 2015

#57

tomxor added a commit that referenced this pull request Jan 5, 2015
Nullify unresolved links (optional)
@tomxor tomxor merged commit dadde81 into master Jan 5, 2015
@tomxor tomxor deleted the iss57 branch January 5, 2015 10:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) when pulling 6f587b4 on iss57 into d8123e3 on master.

@neonichu
Copy link
Contributor

neonichu commented Jan 5, 2015

FWIW, the iOS SDK indeed has blank objects for this case, and a fetched property to indicate if an instance is blank or not: http://cocoadocs.org/docsets/ContentfulDeliveryAPI/1.4.4/Classes/CDAResource.html#//api/name/fetched

It is not possible to distinguish whether is is indeed a dead link or simply one not included in the response via that property, though.

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.

4 participants