-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add updated at and created at to index #170
Add updated at and created at to index #170
Conversation
You can see the unit tests fail. Also, when I run the project locally when I get the following error when trying to create a new index:
I am kinda confused and stuck with this, I thought the server would already be updated and ready for these properties? Any guidance would be appreciated. /cc @curquiza could you be of any assistance? I will also post this question in the Slack for help of other (community) members :) |
Carolina told me I could also ping /cc @alallema |
Hi @sander1095, |
Thanks, that might just give me enough info to look further into this! Expect more updates this evening or perhaps this weekend |
…ub.com/sander1095/meilisearch-dotnet into add-updated-at-and-created-at-to-index
Hello @alallema ! I have been able to resolve the issue, thanks to your quick reply! I am curious about your review! I know Carolina said you people are very busy, but I can't resist asking 🙈 ! Thanks for the opportunity to create this PR! It was fun! :) |
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.
Can you also updat UpdateIndex()
in the Index class as you updated FetchInfo()
? :) Thanks
bors try |
tryBuild failed: |
looks like the tests do not pass anymore, small changes might be needed :) |
@curquiza I have made the requested changes 🥳 |
🔒 Permission denied Existing reviewers: click here to make sander1095 a reviewer |
I tried doing |
bors try |
tryBuild succeeded: |
I also decided to not override the camelcase property if a user specified different JSON settings because they might override them for a specific reason. OVerriding their choices seem counter intuitive
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.
Please don't modify PrepareJsonPayload
, we don't want the IgnoreNullValues
option be applied to every call :)
Also, it seems to have some git conflict you need to solve before I can review again!
thank you again for your involvement!
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.
Thank you so much @sander1095
bors merge
I think I already sent the form link, but just in case:
If you are participating in Hacktoberfest, and you would like to receive a small gift from MeiliSearch too, please complete this form.
bors merge- |
Canceled. |
bors merge |
170: Add updated at and created at to index r=curquiza a=sander1095 This PR attempts to add the `UpdatedAt` and `CreatedAt` properties to the dotnet library. Resolves #162 Co-authored-by: Sander ten Brinke <s.tenbrinke2@gmail.com> Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
170: Add updated at and created at to index r=curquiza a=sander1095 This PR attempts to add the `UpdatedAt` and `CreatedAt` properties to the dotnet library. Resolves #162 Co-authored-by: Sander ten Brinke <s.tenbrinke2@gmail.com> Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
Since bors seems to be broken I merge manually. |
170: Add updated at and created at to index r=curquiza a=sander1095 This PR attempts to add the `UpdatedAt` and `CreatedAt` properties to the dotnet library. Resolves #162 Co-authored-by: Sander ten Brinke <s.tenbrinke2@gmail.com> Co-authored-by: Clémentine Urquizar <clementine@meilisearch.com>
This PR attempts to add the
UpdatedAt
andCreatedAt
properties to the dotnet library.Resolves #162