joukevandermaas / saule

JSON API library for ASP.Net Web API 2.
https://joukevandermaas.github.io/saule
MIT License
76 stars 37 forks source link

Optional links support #153

Closed bjornharrtell closed 7 years ago

bjornharrtell commented 7 years ago

Aims to resolve #150.

Not yet finished though but wanted to ask about the approach. I couldn't see it possible to handle this in the UrlPathBuilder but had to involve the ResourceSerializer. Also, I'm not sure about what a "canonical" url is and so have problems understanding the purpose of Related vs RelatedCanonical.

bjornharrtell commented 7 years ago

Thanks @joukevandermaas, good to know I'm hopefully on the right path. Will ping you when I have had the time to make this PR complete and properly reviewable and will take your early comments into consideration.

bjornharrtell commented 7 years ago

Improved now but amount of tests still too few and should try it out on a real API implementation too.

bjornharrtell commented 7 years ago

Additional tests added and seems to do what I expect in a real project. Ready for review now @joukevandermaas.

But... I did notice a possible regression with the new serializer. Using 1.7.0.390 I'm missing the data property for relationships (only tested BelongsTo as of yet), downgrading to stable 1.6 resolves it.

laurence79 commented 7 years ago

@bjornharrtell damn... could you give me an example or even better, a test, and I'll investigate the regression?

bjornharrtell commented 7 years ago

@laurence79 will try to produce a unit test to demonstrate the issue.

bjornharrtell commented 7 years ago

See testcase in https://github.com/joukevandermaas/saule/pull/155.

joukevandermaas commented 7 years ago

I like the changes! I think this can be merged once the documentation has been updated (I think docs/generating-links.md is a good spot).

bjornharrtell commented 7 years ago

Great, will add docs/generating-links.md asap. But what about RelatedCanonical? Shall I remove that one?

bjornharrtell commented 7 years ago

Added documentation and tentatively removed RelatedCanonical enum type.

joukevandermaas commented 7 years ago

Thanks for seeing this through! It turned out really nice, I think.