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

Remove unnecessary dependencies #6

Merged
merged 2 commits into from Oct 22, 2018
Merged

Remove unnecessary dependencies #6

merged 2 commits into from Oct 22, 2018

Conversation

@fschutt
Copy link
Contributor

fschutt commented Sep 24, 2018

  • arrayvec isn't used at all, it's just a baggage dependency
  • failure is only used for implementing Display and for saving 20 lines, you import all of these dependencies:
├── failure v0.1.2
│   ├── backtrace v0.3.9
│   │   ├── cfg-if v0.1.5
│   │   ├── rustc-demangle v0.1.9
│   │   └── winapi v0.3.6 (*)
│   └── failure_derive v0.1.2
│       ├── proc-macro2 v0.4.19 (*)
│       ├── quote v0.6.8 (*)
│       ├── syn v0.14.9
│       │   ├── proc-macro2 v0.4.19 (*)
│       │   ├── quote v0.6.8 (*)
│       │   └── unicode-xid v0.1.0 (*)
│       └── synstructure v0.9.0
│           ├── proc-macro2 v0.4.19 (*)
│           ├── quote v0.6.8 (*)
│           ├── syn v0.14.9 (*)
│           └── unicode-xid v0.1.0 (*)

If that's worth it... well, I don't know. You decide.

  • In the future, dwrote-rs currently depends on serde (which should be optional, as pathfinder doesn't use any serde features, so it's just a baggage dependency). A future PR should remove that dependency, too (serde is pretty large).

This PR brings the dependency count from 44 to 38, removing 6 dependencies with no features removed or API changed.

@fschutt
Copy link
Contributor Author

fschutt commented Sep 24, 2018

If servo/dwrote-rs#21 should be merged, the number of dependencies falls to 31 (- 13 dependencies) if serde is disabled. The reason for this is that failure and serde pull in the whole quote, syn and proc-macro dependencies, which are only used for syntax sugar.

syn is actually required two times (and has to be compiled twice), because 0.14 and 0.15 can't be used together. failure takes extremely long to build. If I run time cargo build, I get this result:

Before: 0m31,691s
After: 0m20,051s

I hope you see why I'm after reducing dependencies as far as possible.

@pcwalton pcwalton merged commit d773511 into servo:master Oct 22, 2018
@ashkitten
Copy link
Contributor

ashkitten commented Jan 2, 2019

@fschutt when you removed failure you never implemented std::error::Error for the error types, and therefore made them incompatible with the try! macro or ? operator

@fschutt
Copy link
Contributor Author

fschutt commented Jan 3, 2019

@ashkitten It's not incompatible with the try operator, the try operator has nothing to do with std::error::Error. Second, that was on purpose, because std::error::Error does not allow to dynamically format error messages such as https://github.com/pcwalton/font-kit/pull/6/files#diff-9a269209bed069ffb36d5565a91c9840R52 - std::error::Error only allows to return a &'static str, which is why it's a pretty awful trait for errors in general (because you can't have dynamic information in error messages).

The error implements Debug + Display, so you can easily wrap it in your own error type and implement std::error::Error for that if necessary.

@ashkitten
Copy link
Contributor

ashkitten commented Jan 3, 2019

alright then. i do think it's ridiculous to ask someone to wrap your errors in their own type just to be able to use them, feels like going back to the error-chain days with a 200 line enum to wrap all possible errors of every crate you use

@fschutt
Copy link
Contributor Author

fschutt commented Jan 3, 2019

feels like going back to the error-chain days with a 200 line enum to wrap all possible errors of every crate you use

Well, that's the proper way to do errors though, Box<Error> makes you lose any type information about the error, so it's not better that a type-erased exception. I've elaborated on why I think std::error::Error is one of the worst traits in the standard library here - for displaying errors, just use Debug and Display instead.

@ashkitten
Copy link
Contributor

ashkitten commented Jan 7, 2019

@fschutt i've had time to think more about this and consult others and i think it's ridiculous to not implement std::error::Error for your errors. making other people do extra work in applications that don't need extra type information from errors is just you pushing your personal preferences onto others. any type that is used in the Err position of Result should implement std::error::Error.

@fschutt
Copy link
Contributor Author

fschutt commented Jan 7, 2019

@ashkitten Well, if you do feel strongly about this then fine, make a PR and implement it. I'm just saying that it's duplicated code since Display is more flexible in terms of formatting and Box<Error> will hurt you in the long run. If you still want to do it, go ahead.

The thing I'd oppose to, however, is to use an external library such as failure to implement std::error::Error, because you'd import 16 extra dependencies to avoid writing three lines of extra code. With a larger project it's already hard to keep dependencies low and build times short and you wouldn't be helping the cause there. That's all I'm asking, to keep the dependency count low.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.