Remove redundant declarations of `+ (instancetype)repository`

[hideya]

Remove redundant declarations of + (instancetype)repository scattered around LBModelRepository's subclasses. This clean-up is mainly for the codegen work.

@raymondfeng , would you please review the changes?

[bajtos]

IIUC, this change is moving the declaration of + (instancetype)repository to a base class, with a default implementation returning null, and it's fully backwards compatible.

I'm little bit worried that developers manually extending LBModel may forget to override this default implementation and they won't realise their mistake until they hit a null pointer exception at runtime. Is it a valid concern? Is there a way how to let developers extending LBModel class aware of the fact that they need to override the repository property?

Is it worth adding a unit-test for this change?

This clean-up is mainly for the codegen work.

Could you please explain in more detail why is this needed by the codegen?

[bajtos]

@raymondfeng I am taking over the review of this PR, please do speak up if you object.

[hideya]

@bajtos thank you for your careful review! I wish Obj-C had a support for abstract method...

IIUC, this change is moving the declaration of + (instancetype)repository to a base class, with a default implementation returning null, and it's fully backwards compatible.

Yes, that's correct.

I'm little bit worried that developers manually extending LBModel may forget to override this default implementation and they won't realise their mistake until they hit a null pointer exception at runtime.

Good point. I'll make +[LBModelRepository repository]'s default implementation throw an exception with explanations (it is not yet detectable at compile time, but far better than just returning a nil).

Is it worth adding a unit-test for this change?

Once I made the above change, it may be beneficial to add a unit test to check for an exception when 'repository' method is not overridden (although it is not that critical).

Could you please explain in more detail why is this needed by the codegen?

Sorry for the short and half-way note. It is not mandatory for the codegen, but it makes generated code slightly cleaner and easier to understand, I thought. The method + (instancetype)repository should really be a part of LBModelRepository's API (considering internal dependency on it) but currently it is added to each subclass by convention (i.e. if a developer forget to add the method, the execution fails somewhere internally by "no such method"). I realized this while working on the codegen and thought to improve the situation somehow. Does this make sense?

[hideya]

I just made +[LBModelRepository repository] throw an exception and added a unit test to check this behavior of throwing an exception if not overridden.

[bajtos]

I'm little bit worried that developers manually extending LBModel may forget to override this default implementation and they won't realise their mistake until they hit a null pointer exception at runtime.

Good point. I'll make +[LBModelRepository repository]'s default implementation throw an exception with explanations (it is not yet detectable at compile time, but far better than just returning a nil).

Interesting. I have one concern about thrown an exception. IIRC, the SDK allows users to use LBPersistedModel directly, without creating a subclass for each of their server models. Will be this behaviour preserved?

If LBPersistedModel does not throw and returns null instead, then we have still have the problem when the SDK user decides to actually create a LBPersistedModel subclasses for their server models.

I feel this may be a language constraint and there may not be any single solution good for all consumers. If that's the case, then I think I prefer the ability to use LBPersistedModel directly without subclassing.

Thoughts?

[bajtos]

The method + (instancetype)repository should really be a part of LBModelRepository's API (considering internal dependency on it) but currently it is added to each subclass by convention (i.e. if a developer forget to add the method, the execution fails somewhere internally by "no such method").

I see, now the change makes sense. I don't have enough ObjC experience, so tell me, which runtime error message makes it easiest for the developer to identify and fix the problem - the one about missing method, or the explicit error you are raising now, or a null pointer exception raised by the property returning null? I think we should take that into consideration too.

[hideya]

the SDK allows users to use LBPersistedModel directly, without creating a subclass for each of their server models. Will be this behaviour preserved?

Yes. Unit tests LBPersistedModelTests actually test such a case (and LBPersistedModelSubclassingTests are for cases where LBPersistedModel gets subclassed). The way to create a repository is different (-[LBRESTAdapter repositoryWithModelName:] vs -[LBRESTAdapter repositoryWithClass:]) and the former doesn't depend on +[LBModelRepository repository].

which runtime error message makes it easiest for the developer to identify and fix the problem

I'd say adding +[LBModelRepository repository]method and letting it throw an explicit exception would be easiest to identify and fix the problem. The error message tells developers exactly what is wrong, and +[LBModelRepository repository] will be shown as a part of LBModelRepository class reference, where developers can get further information.
If I could break the backward compatibility, I would consider to use 'protocol', which makes missing repository detectable at compile time, but it requires updating all the prototypes of LBModelRepository subclasses and thought it'd be too much at the moment...

[bajtos]

@hideya thank you for the explanation. I don't have enough capacity to think this fully through again. Your response makes sense, so let's do what you are proposing.

The current patch (as of 04dd702) LGTM. I'd personally merge both commits to a single one and fix the commit message to follow 50/72 rule (the first commit has too long summary line), but I can live without that too.

Feel free to land this patch when you consider it good to go.

[hideya]

@bajtos thanks for the comment back. I've combined the two commits and am going to merge the PR.