-
Notifications
You must be signed in to change notification settings - Fork 44
404 sync api v2 #405
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
404 sync api v2 #405
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 93.28% 88.21% -5.07%
==========================================
Files 130 144 +14
Lines 2783 3021 +238
Branches 350 360 +10
==========================================
+ Hits 2596 2665 +69
- Misses 179 216 +37
- Partials 8 140 +132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
274cca0 to
d3c8712
Compare
| /// <summary> | ||
| /// Initializes synchronization of changes in content items, content types, taxonomies or languages based on the specified parameters. After the initialization, you'll get an X-Continuation token in the response. | ||
| /// </summary> | ||
| /// <param name="parameters">A collection of query parameters, for example, for filtering.</param> |
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 guess this method docs were copypasted from v1, because the method doesn't have any params, so the summary should be adjusted and param entry removed altogether
| using System.Threading.Tasks; | ||
| using System.Threading.Tasks; | ||
|
|
||
| namespace Kontent.Ai.Delivery.Abstractions |
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 don't see GetSyncAsync or GetSyncV2Async in the extension methods. there are other methods present in client which are not provided through the extensions, so this is fine by me, just wanted to point it out for consideration, whether it would make sense to have this here too.
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 think since there will be no params the post is redundant and I have removed it
| namespace Kontent.Ai.Delivery.Sync | ||
| namespace Kontent.Ai.Delivery.Sync; | ||
|
|
||
| internal sealed class SyncItemData : ISyncItemData |
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 see this class was inherited from v1 of sync api, but is there a reason why it's internal sealed, while all other classes related to sync (SyncV2Language, SyncV2ContentType) are public?
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.
there was just a switch to file scoped namespace in this class
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.
other classes fixed
| /// Initializes a new instance of <see cref="SyncV2Item"/> class. | ||
| /// </summary> | ||
| [JsonConstructor] | ||
| public SyncV2Item(object stronglyTypedData, ISyncV2ItemData data, string changeType, DateTime timestamp) |
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.
unused constructor param stronglyTypedData
|
|
||
| namespace Kontent.Ai.Delivery.SyncV2 | ||
| { | ||
| internal sealed class SyncV2ItemData : ISyncV2ItemData |
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.
again, should this be internal sealed and not public (or other way around)?
| /// Initializes a new instance of <see cref="SyncV2Language"/> class. | ||
| /// </summary> | ||
| [JsonConstructor] | ||
| public SyncV2Language(object stronglyTypedData, ISyncV2LanguageData data, string changeType, DateTime timestamp) |
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.
unused constructor param stronglyTypedData
| /// Initializes a new instance of <see cref="SyncV2Taxonomy"/> class. | ||
| /// </summary> | ||
| [JsonConstructor] | ||
| public SyncV2Taxonomy(object stronglyTypedData, ISyncV2TaxonomyData data, string changeType, DateTime timestamp) |
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.
unused constructor param stronglyTypedData
| /// Generates a URL for sync v2 initialization. | ||
| /// </summary> | ||
| /// <param name="parameters">Filtering parameters.</param> | ||
| /// <returns>A valid URL containing correctly formatted parameters.</returns> |
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.
again, no filtering params for this function, even though its docs suggest otherwise.
| } | ||
|
|
||
| [Fact] | ||
| public async Task SyncV2Api_PostSyncV2InitAsync_WithParameters_GetContinuationToken() |
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.
misleading function name, tested method does not accept any parameters
| /// <summary> | ||
| /// Represents a response from Kontent.ai Sync API. Response includes continuation token for subsequent synchronization calls. Sync initialization should always return an empty list. | ||
| /// </summary> | ||
| public interface IDeliverySyncV2InitResponse : IResponse |
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 get that sync init should always return an empty list, but since IDeliverySyncV2InitResponse and IDeliverySyncV2Response are identical, is this duplicity needed for anything? perhaps future updates? otherwise, I'd just get rid of the duplicit interface definition
Motivation
Fixes #404
Checklist
How to test
Verify the client can be used with the sync v2 endpoints