meilisearch / meilisearch-rust

Rust wrapper for the Meilisearch API.
https://www.meilisearch.com
MIT License
361 stars 90 forks source link

Update dependencies #542

Closed omid closed 8 months ago

omid commented 9 months ago

Update dependencies and a bit of housekeeping.

omid commented 9 months ago

There is a typo in Cancelation (like in TaskCancelation), but it's also like this in the app itself, so it's bigger than the sdk and most probably a breaking change!

curquiza commented 9 months ago

Hello @omid thanks for the PR

We are ok with breaking changes since the app is not stable (using version format like v0.X.Y and not vX.Y.Z), so we can accept a PR. The most important is to be clear in the communication

curquiza commented 9 months ago

Also @omid you said you updated the dependencies but you also changed a lot of things

I would rather we separate the typo fixes and the dependencies updates/code changes that are not related to typos please 😊 I appreciate the work, but it's not really convenient to review and I don't feel comfortable accepting a PR where I have that much files to review for different reasons

omid commented 9 months ago

@curquiza Thanks 🙏🏼

Also @omid you said you updated the dependencies but you also changed a lot of things

I updated syn to 2, jsonwebtoken to 9, and removed env_logger, since it wasn't in use.

I don't feel comfortable accepting a PR where I have that much files to review for different reasons

If you don't mind, the update dep thingy changes are in just one commit which you can find here.

We are ok with breaking changes since the app is not stable (using version format like v0.X.Y and not vX.Y.Z), so we can accept a PR. The most important is to be clear in the communication

Sure, the only issue will be that the change should be applied everywhere! Including all SDKs I think, which I'm not familiar with their languages and it's a big chunk of work also. (github said 50 files in the meilisearch org)

curquiza commented 9 months ago

If you don't mind, the update dep thingy changes are in just one commit which you can find here.

Not 100% true, this commit also change some parts of the code at the middle of the non-code related update: https://github.com/meilisearch/meilisearch-rust/pull/542/commits/351651951e50155d4125a6df4ec23b914cfd1e0a

I really would rather you separate your changes, so your PRs, please. For our convenience. We have a lot of things to review, and not a lot of time, it will really help us 🙏

curquiza commented 9 months ago

Sure, the only issue will be that the change should be applied everywhere! Including all SDKs I think, which I'm not familiar with their languages and it's a big chunk of work also. (github said 50 files in the meilisearch org)

Can you point which kind of change you refer and why the issue is in all the SDKs?

omid commented 9 months ago

Can you point which kind of change you refer and why the issue is in all the SDKs?

As I said in the first post here, There is a typo in *Cancelation*. It's wrong, it's cancellation, double LL.

omid commented 9 months ago

Not 100% true, this commit also change some parts of the code at the middle of the non-code related update:

The update deps are in one commit. Others are typos and some housekeeping. There are some changes in the macro part in other commits, but they are not about the logic. Just code style, typo and so on.

curquiza commented 9 months ago

Can you fix git conflicts?

meili-bors[bot] commented 8 months ago

Build succeeded: