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

Implement the provided operators for Rgb manually #13

Merged
merged 2 commits into from
Oct 12, 2022
Merged

Implement the provided operators for Rgb manually #13

merged 2 commits into from
Oct 12, 2022

Conversation

nickelc
Copy link

@nickelc nickelc commented Oct 12, 2022

This removes the overload dependency by implementing the operators for the Rgb struct manually.

The overload macro is a bit opaque about what it generates without looking into the documentation. The generate code is fairly simple and therefore I think it's reasonable to maintain the implementation here.

Copy link
Member

@sholderbach sholderbach left a comment

Choose a reason for hiding this comment

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

I am generally on board with trying to keep the depency footprint as small as possible

In this case the effect on build times is neglible as the overload crate is entirely build with macro_rules! macros instead of being a proc macro crate where the proc_macro2/quote/syn compile chain has a big impact.

Output from cargo clean && cargo build --timings:

grafik

src/rgb.rs Outdated Show resolved Hide resolved
@nickelc
Copy link
Author

nickelc commented Oct 12, 2022

I'm aware that it's just a macro_rules! and that the build times are negligible.

I saw new transient dependencies popping up in my project, checked them out and
wouldn't mind if the macro was used for more than one type.

@fdncred
Copy link

fdncred commented Oct 12, 2022

+1 for readability

@sholderbach
Copy link
Member

I'm on board with keeping it straightforward over saving a few lines of boilerplate.

Thanks for taking up the chores!

@sholderbach sholderbach merged commit 7fdaa4a into nushell:main Oct 12, 2022
@nickelc nickelc deleted the less-deps branch October 12, 2022 12:58
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