-
Notifications
You must be signed in to change notification settings - Fork 59
Implement Src resource #104
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
Conversation
…on to simplify usage four boundary cases of null or empty directory names (to target the root directory or the current root directory)
…that have a query string This allow to fix the SrcResource test that were ignored due to that issue
…re revision is not provided
…rcResource listing
| public class TreeEntryLinks | ||
| { | ||
| public Link Self { get; set; } | ||
| public Link Meta { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The meta property was not declared in the existing Links POCO.
Since TreeEntry responses returns only two links I've created a new POCO that represent the only two links that are returned in a TreeEntry, but we can also choose to add a Meta property in the existing Links class since there is already a lot of cases where we manipulate POCOs that won't be entirely filled.
What should we do in general ?
- Have POCOs that cover all the possible properties in the various scenarios ?
- Or have a lot of specialized POCOs that are tied to each scenario ?
In the first case, we will have less objects and potentially allow the consumers to easily complete and reuse POCOs without doing mapping between similar objects.
In the second case, we will have more objects, but consumers will be less tempted to think that they can found some data in some contexts. However it may happens that consumers will have to do some mappings between similar POCOs.
I asked because the question is the same for the Commit property of the TreeEntry POCO.
There, I've used an existing POCO, but in fact it's more a commit reference than a full commit POCO, since we only get the hash and the link that allow to get the full commit POCO.
So it would be better to have a same strategy for the Links property than for the Commit property and choose between reusing POCOs or declaring specialized POCOs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I've tried to expose the facts I'll give my opinion:
I prefer the second case because like that we always know exactly what we consume, and because use cases where we have to perform some mappings should be rare. And even when mappings are required, by experience, we often want to extract just few fields and drop a lot of others.
Reusing POCOs has always appear to me as a lazy way since it spares you to find names for similar POCOs (like commit / CommitReference or Links / TreeEntryLinks) and it spares you to think of an inheritance model (like: does Commit POCO should inherit from Commit Reference ?)
But a lazy approach could also be a valid approach for a project where we don't want to invest too much time...
And you @MitjaBezensek, what's your opinion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've found in the bitbucket documentation a paragraph about their point of view Condensed Versus Full Objects
So for them what I've called 'specialized POCO" or "Reference to another object" is called a "condensed representation"
I've also discovered that some specialized POCOs for condensed form of the objects have already been created:
- ProjectInfo
- PullRequestInfo
- UserShort
So I think that the best solution is to continue with the "{type}Info" pattern, and document that clearly somewhere in our documentation that it's the pattern naming for the condensed form of the objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree with you and I'm all for specialized POCOs. I tried to use the Info pattern where I saw the need for it.
That way the end users know exactly what they are getting. Otherwise we might start getting support questions of the type "Why is this field empty when I call this?".
# Conflicts: # Coverage.md => coverage of Src resource has been rewritten with the new format of the document
also transform the TreeEntry POCO to camel case since it's what is done in all other POCOs and add the history link in the TreeEntryLinks
… on the content of the POCOs in multiple cases This allow to bring the light on the differences between Files and Directories relative to how the TreeEntry POCO is filled
…ectory POCOs Those have been designed to be more intuitive to consume since there is a clear separation of the properties that are accessible for files and directories
Implements the Src resource https://developer.atlassian.com/bitbucket/api/2/reference/resource/repositories/%7Busername%7D/%7Brepo_slug%7D/src#get
Only GET operations have been implemented since the POST operation is really particular.
The POST allow to create commits. So why it's in Src and not in Commit resource ?
And it use
multipart/form-datawhile our implementation useapplication/jsonto post data for API V2 in RequestExecutorV2 which means that it will require important changes and probably breaking changes to be able to specify how post operations should be performed.