-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature-gate tests to reduce dependencies #108
Conversation
Hi Ryan, thanks for the PR — I like the idea! I'll try to find time to review it tomorrow. |
See added comments for rationales behind each stage
Hey again, Thanks for updating now that you can run the CI tests.
Okay, these are the features in the PR:
Since there are so many crates in play, I would probably say the simplest approach would be to name the features after the macros? So
I would suggest turning them all on by default so that thinks keep working the way people are used to. I like to offer people features first, and speed second, if that makes sense :) I've done something similar in my Textwrap library and there I added a section to the top-level API doc to explain what the features do: https://docs.rs/textwrap/0.14.2/textwrap/index.html#cargo-features. I would like to add something similar here — if not in this PR, then later. Let me add some comments to the changes themselves as well. |
Alternative would be finding a way to write valid powershell+bash simultaneously, which would be very cursed
I addressed some of these changes, and am now thinking out how to simplify/move the conditional imports and how to write up a blurb in the documentation describing the features. |
Thanks, sounds good! |
@mgeisler I think this is ready to merge now. Could you please take a look and either merge it or point to other things that you think need changing? |
Yes, thank you so much! I think it looks very good too :-) It's Friday evening in my timezone, so I'll look it over and squash merge it tomorrow. Btw, your step-by-step commits look great and I think the tests pass for all of them except the first few. Of you want, you could rework the first commits so that tests pass and then I'll be happy to merge your branch. Otherwise I'll just squash merge it. |
I'd rather preserve commit history instead of reworking past commits, so I'd prefer that you squash-merge it (and I'll keep my branch up on my fork for people who'd want to go through the commit history). |
Okay, sounds good! Thanks for all the updates based on my comments. Would you be up for adding a |
Unfortunately I'll be busy for the next few days, so I think it'd be better for you to do it yourself. |
This PR adds three features, one for each macro, and allows a user who only needs one macro to avoid dependencies that are only needed for the other macros.
Feedback requested: