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 two dependencies #28

Merged
merged 3 commits into from
Nov 24, 2024
Merged

Conversation

FreezyLemon
Copy link
Contributor

Remove thiserror and paste. Both are proc-macros, but syn is still required because of av-data using num-derive in a few places.

Still, I think these are simple changes (without API breakage) and fewer dependencies are good.

@shssoichiro
Copy link
Collaborator

I have mixed feelings about this, because of the increased boilerplate. Let me think about it... or convince me. 🤷‍♂️

@FreezyLemon
Copy link
Contributor Author

You mean because the #[error("...")] syntax is nicer than the manual Error implementation? I can see that.. I've made it a habit to avoid proc-macros unless they pull their weight. Maybe it was overzealous in this case, because we can't avoid syn anyways with av-data still pulling it in. And I've gotten used to the boilerplate, I guess.

I don't really have a strong stance on this. If you think it's not worth it, that's fine.

@shssoichiro
Copy link
Collaborator

That as well as the duplication from removing paste. What impact does it have on the compilation time? Maybe not as much as we'd prefer since we still need syn at the moment.

@FreezyLemon
Copy link
Contributor Author

A fresh build on my notebook is a tiny bit quicker (0.5s / 10%)...

I tried replacing num-derive (last syn-based dependency) in av-data a few months ago, but that was understandably rejected. The relevant commit is still accessible, but as you can see, the code for that is an arguable downgrade. I guess we may need to wait for something like rust-lang/rfcs#3698

@shssoichiro
Copy link
Collaborator

We may be waiting a long time for that. In the meantime, I suppose this change isn't that bad, since it's just a small amount of boilerplate.

@shssoichiro shssoichiro merged commit b156513 into rust-av:main Nov 24, 2024
3 checks passed
@FreezyLemon FreezyLemon deleted the reduce-dependencies branch November 24, 2024 19:21
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.

2 participants