Skip to content

Conversation

@rydrman
Copy link
Collaborator

@rydrman rydrman commented Jan 23, 2023

The PkgRequest type was using thiserror::Error, which doesn't seem appropriate or necessary to me. It seems like this was added in order to get the Display trait, or at least it was only being used for that, so I've added an explicit implementation and removed the derive.

Signed-off-by: Ryan Bottriell <ryan@bottriell.ca>
@rydrman rydrman added this to the V1 Spec milestone Jan 23, 2023
@rydrman rydrman requested review from dcookspi and jrray January 23, 2023 19:26
@rydrman rydrman self-assigned this Jan 23, 2023
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #631 (627ef5d) into master (6a3fc42) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   57.61%   57.58%   -0.04%     
==========================================
  Files         215      215              
  Lines       15206    15211       +5     
==========================================
- Hits         8761     8759       -2     
- Misses       6445     6452       +7     
Impacted Files Coverage Δ
crates/spk-schema/crates/ident/src/request.rs 80.95% <0.00%> (-1.41%) ⬇️
crates/spk-solve/crates/graph/src/error.rs 0.00% <ø> (ø)
crates/spfs/src/storage/repository.rs 76.74% <0.00%> (-2.33%) ⬇️
crates/spfs/src/storage/fs/hash_store.rs 72.32% <0.00%> (-0.63%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rydrman rydrman mentioned this pull request Jan 23, 2023
11 tasks
Copy link
Collaborator

@jrray jrray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI graph::Graph also derives Error.

@rydrman rydrman merged commit 2410a88 into master Feb 1, 2023
@rydrman rydrman deleted the 296-remove-err-impl-pkg-request branch February 1, 2023 00:41
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.

3 participants