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

Automatically include dependencies #37

Merged
merged 1 commit into from
Nov 25, 2023
Merged

Conversation

tamird
Copy link
Contributor

@tamird tamird commented Nov 19, 2023

  • Move macros to a new test-log-macros crate.
  • Export env_logger and tracing_subscriber from the test-log crate.
  • Macros emit test_log::{env_logger,tracing_subscriber} references.

Removes the need for users to explicitly depend on additional crates.

Fixes #36.

@tamird tamird force-pushed the macros-crate branch 6 times, most recently from 1f8d999 to 7c2be36 Compare November 19, 2023 19:37
@tamird
Copy link
Contributor Author

tamird commented Nov 19, 2023

Needed some elbow grease to get minimum versions working, but this is green now.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Nov 22, 2023

Very cool! Will take a closer look later this week.

@d-e-s-o d-e-s-o self-requested a review November 24, 2023 21:44
@d-e-s-o d-e-s-o self-assigned this Nov 24, 2023
Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

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

Overall this looks good. I'd prefer we move the macros stuff into macros/ in the workspace root instead of crates/test-log-macros (crate name is fine, though). Can you please revert format changes that snug in as part of the lib.rs code move?
I think we are going to have to adjust the publish workflow to accommodate, but I guess I can take care of that.

The other thing is, with this change users loose the ability to indirectly control additional features of, say, tracing-subscriber. E.g., earlier a project could add

tracing-subscriber = {version = "0.3.17", default-features = false, features = ["ansi", "env-filter", "fmt"]}

and would get colored traces. This no longer works. I don't think it's a deal breaker, but it kind of sucks. But I guess the set of features this applies to is limited, and given that env_logger supports colored output now as well, we may as well expose a color feature ourselves. But that can come later.

Cargo.toml Outdated
@@ -28,27 +28,27 @@ tracing infrastructure before running tests.
include = ["src/lib.rs", "LICENSE-*", "README.md", "CHANGELOG.md"]

[lib]
Copy link
Owner

Choose a reason for hiding this comment

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

Can we remove the empty section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 71 to 72
# Probably fixed by https://github.com/rust-lang-nursery/lazy-static.rs/pull/107.
- run: cargo +nightly update lazy_static --precise 1.0.2
Copy link
Owner

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tamird
Copy link
Contributor Author

tamird commented Nov 25, 2023

Overall this looks good. I'd prefer we move the macros stuff into macros/ in the workspace root instead of crates/test-log-macros (crate name is fine, though).

I had put it in crates so that the workspace specification can use a glob:

[workspace]
members = ["crates/*"]

otherwise this list needs an update any time a new crate is added to the workspace. Let me know if you insist.

Can you please revert format changes that snug in as part of the lib.rs code move?

Done. That snuck in because I ran cargo fmt on stable, which doesn't know about some of the options used in this project.

I think we are going to have to adjust the publish workflow to accommodate, but I guess I can take care of that.

The other thing is, with this change users loose the ability to indirectly control additional features of, say, tracing-subscriber. E.g., earlier a project could add

tracing-subscriber = {version = "0.3.17", default-features = false, features = ["ansi", "env-filter", "fmt"]}

and would get colored traces. This no longer works. I don't think it's a deal breaker, but it kind of sucks. But I guess the set of features this applies to is limited, and given that env_logger supports colored output now as well, we may as well expose a color feature ourselves. But that can come later.

I've changed the optional dependencies version specs to use >= so that the use case you describe still works. See the comment I added.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Nov 25, 2023

I had put it in crates so that the workspace specification can use a glob:

[workspace]
members = ["crates/*"]

otherwise this list needs an update any time a new crate is added to the workspace. Let me know if you insist.

I think it's fine to update the list when a new member is added and would go so far to say that listing each explicitly is, well, more explicit. I just can't get myself to liking the crates/ container directory, though I acknowledge that it's subjective and that a bunch of projects use that.

I've changed the optional dependencies version specs to use >= so that the use case you describe still works. See the comment I added.

To be honest, it seems to work with and without your changes now. No idea what I did or didn't do yesterday. Regarding your change: Will this mean that if tracing-subscriber releases an incompatible 0.4 that everything may break? If so, I'd prefer we stick to the default semver requirements to be on the safe side. Sorry for the confusion.

- Move macros to a new test-log-macros crate.
- Export env_logger and tracing_subscriber from the test-log crate.
- Macros emit test_log::{env_logger,tracing_subscriber} references.

Removes the need for users to explicitly depend on additional crates.

Fixes d-e-s-o#36.
@tamird
Copy link
Contributor Author

tamird commented Nov 25, 2023

I had put it in crates so that the workspace specification can use a glob:

[workspace]
members = ["crates/*"]

otherwise this list needs an update any time a new crate is added to the workspace. Let me know if you insist.

I think it's fine to update the list when a new member is added and would go so far to say that listing each explicitly is, well, more explicit. I just can't get myself to liking the crates/ container directory, though I acknowledge that it's subjective and that a bunch of projects use that.

Done.

I've changed the optional dependencies version specs to use >= so that the use case you describe still works. See the comment I added.

To be honest, it seems to work with and without your changes now. No idea what I did or didn't do yesterday. Regarding your change: Will this mean that if tracing-subscriber releases an incompatible 0.4 that everything may break? If so, I'd prefer we stick to the default semver requirements to be on the safe side. Sorry for the confusion.

Done.

@d-e-s-o d-e-s-o merged commit c94977b into d-e-s-o:main Nov 25, 2023
6 checks passed
@tamird tamird deleted the macros-crate branch November 25, 2023 16:59
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.

features should not require users to manually include dependencies
2 participants