Skip to content
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

Error enum fixed #3487

Merged
merged 34 commits into from
Jul 10, 2023
Merged

Error enum fixed #3487

merged 34 commits into from
Jul 10, 2023

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Jul 5, 2023

Replaces #3304. There were a number of problems with that one, most importantly error responses being formatted incorrectly so that api tests would fail. Also some errors were renamed, eg "couldnt -> could not" which would break the api. This should all be fixed now.

@Nutomic Nutomic requested a review from dessalines as a code owner July 5, 2023 10:59
@Nutomic Nutomic mentioned this pull request Jul 5, 2023
@SleeplessOne1917
Copy link
Member

I'll have to update the js client PR whenever this is merged.

@phiresky
Copy link
Collaborator

phiresky commented Jul 5, 2023

this is great. maybe you could impl From<LemmyErrorType> for LemmyError though? so that all those LemmyError::from_type(LemmyErrorType::CantBlockAdmin) are shorter. then you should even be able to just use the ? operator on lemmyerrortype in many cases

@phiresky
Copy link
Collaborator

phiresky commented Jul 5, 2023

You should also be able to make the from_error_and_type method an extension method on Into<anyhow::Error>, then this code:

  PostRead::mark_as_unread(pool, &post_read_form)
    .await
    .map_err(|e| LemmyError::from_error_and_type(e, LemmyErrorType::CouldntMarkPostAsRead))
}

would look like the following:

  PostRead::mark_as_unread(pool, &post_read_form)
    .await
    .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)
}

@Nutomic
Copy link
Member Author

Nutomic commented Jul 5, 2023

Good idea, Ive implemented impl From<LemmyErrorType> for LemmyError and it really shortens t he code.

But I dont know how the extension method is supposed to work, Ive never used that before. I tried with this code:

pub trait WithLemmyType: Into<anyhow::Error> {
  fn with_lemmy_type(self, error_type: LemmyErrorType) -> LemmyError {
    LemmyError::from_error_and_type(self, error_type)
  }
}

impl<T: Into<anyhow::Error>> WithLemmyType for T {}

And calling it exactly like in your example. But it fails with the following error, seems like the method should really take Result, but not sure how to do that.

)
error[E0599]: the method `with_lemmy_type` exists for enum `Result<usize, Error>`, but its trait bounds were not satisfied
--> crates/api_common/src/utils.rs:134:6
|
132 | /   PostRead::mark_as_unread(pool, &post_read_form)
133 | |     .await
134 | |     .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)
| |     -^^^^^^^^^^^^^^^ method cannot be called on `Result<usize, Error>` due to unsatisfied trait bounds
| |_____|
|
|
::: /home/felix/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/result.rs:503:1

@Nutomic Nutomic marked this pull request as draft July 5, 2023 14:20
@phiresky
Copy link
Collaborator

phiresky commented Jul 5, 2023

here's a working extension trait:

pub trait LemmyErrorExt<T, E: Into<anyhow::Error>> {
  fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result<T, LemmyError>;
}

impl<T, E: Into<anyhow::Error>> LemmyErrorExt<T, E> for Result<T, E> {
  fn with_lemmy_type(self, error_type: LemmyErrorType) -> Result<T, LemmyError> {
    self.map_err(|e| LemmyError::from_error_and_type(e, error_type))
  }
}

it just has to extend Result<T, into anyhow> and not into directly

@Nutomic Nutomic marked this pull request as ready for review July 6, 2023 11:40
@Nutomic
Copy link
Member Author

Nutomic commented Jul 6, 2023

Thats it, got it all cleaned up now.

@Nutomic Nutomic merged commit 93225e5 into main Jul 10, 2023
2 checks passed
PawanHegde pushed a commit to PawanHegde/lemmy that referenced this pull request Jul 10, 2023
* Create error type enum

* Replace magic string slices with LemmyErrorTypes

* Remove unused enum

* Add rename snake case to error enum

* Rename functions

* clippy

* Fix merge errors

* Serialize in PascalCase instead of snake_case

* Revert src/lib

* Add serialization tests

* Update translations

* Fix compilation error in test

* Fix another compilation error

* Add code for generating typescript types

* Various fixes to avoid breaking api

* impl From<LemmyErrorType> for LemmyError

* with_lemmy_type

* trigger ci

---------

Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>
Nutomic added a commit that referenced this pull request Jul 10, 2023
* Fix #3366: API does return plain HTML errors

* Fix Clippy errors

* Improve api response times by doing send_activity asynchronously (#3493)

* do send_activity after http response

* move to util function

* format

* fix prometheus

* make synchronous federation configurable

* cargo fmt

* empty

* empty

---------

Co-authored-by: Dessalines <dessalines@users.noreply.github.com>

* Updating `login.rs` with generic `incorrect_login` response. (#3549)

* Adding v0.18.1 and v0.18.0 release notes. (#3530)

* Update RELEASES.md (#3556)

added instruction to find the location of your docker directory (especially useful for those who used ansible since they never had to setup docker manually)

* Use async email sender (#3554)

* Upgrade all dependencies (#3526)

* Upgrade all dependencies

* as base64

* Adding phiresky to codeowners. (#3576)

* Error enum fixed (#3487)

* Create error type enum

* Replace magic string slices with LemmyErrorTypes

* Remove unused enum

* Add rename snake case to error enum

* Rename functions

* clippy

* Fix merge errors

* Serialize in PascalCase instead of snake_case

* Revert src/lib

* Add serialization tests

* Update translations

* Fix compilation error in test

* Fix another compilation error

* Add code for generating typescript types

* Various fixes to avoid breaking api

* impl From<LemmyErrorType> for LemmyError

* with_lemmy_type

* trigger ci

---------

Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>

* Only update site_aggregates for local site (#3516)

* Fix #3501 - Fix aggregation counts for elements removed and deleted (#3543)

Two bugs were found and fixed:
- previously elements removal and deletion were counted as two separate disappearances
- removing comments did not affect post aggregations

* Use LemmyErrorType also make error_type compulsory

* Add missing import for jsonify_plain_text_errors

* Fix formatting

* Trying to make woodpecker run again

---------

Co-authored-by: phiresky <phireskyde+git@gmail.com>
Co-authored-by: Dessalines <dessalines@users.noreply.github.com>
Co-authored-by: rosenjcb <rosenjcb@gmail.com>
Co-authored-by: nixoye <12674582+nixoye@users.noreply.github.com>
Co-authored-by: dullbananas <dull.bananas0@gmail.com>
Co-authored-by: Nutomic <me@nutomic.com>
Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>
Co-authored-by: Sander Saarend <sander@saarend.com>
Co-authored-by: Piotr Juszczyk <74842304+pijuszczyk@users.noreply.github.com>
@metame metame mentioned this pull request Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants