Allow multiple target-os #187
Conversation
Update not handling. Add flexi logger. Add additional target_os tests
darrell-roberts
force-pushed
the
multiple-target-os-option
branch
2 times, most recently
from
5675bf4
to
3ed2ea7
Compare
darrell-roberts
force-pushed
the
multiple-target-os-option
branch
from
3ed2ea7
to
134c2b1
Compare
darrell-roberts
force-pushed
the
multiple-target-os-option
branch
from
201ed86
to
f04d0e4
Compare
This reverts commit 3c1fcc0.
darrell-roberts
force-pushed
the
multiple-target-os-option
branch
from
eec6dc6
to
53291ea
Compare
There was a problem hiding this comment.
There's been a lot of iteration on this PR, and I know the delay is frustrating. I definitely don't think we need to cover every possible edge case, however I do think it would be valuable—even if only for ourselves as future maintainers—to have a clear documentation of how the new flag works and why. That way, if we lose context but have to come back and re-evaluate, we can be more confident we're considering all of the cases that were originally considered.
| if meta.path().is_ident("not") { | ||
| debug!("encountered not"); | ||
| scope = TargetScope::Reject | ||
| } |
There was a problem hiding this comment.
Should this take into account the existing scope? I.e., if we're already in TargetScope::Reject for this node, would not turn that into TargetScope::Accept? It's super edge-case since I don't know that someone would realistically do this, but how do we handle #[cfg(not(not(target_os = "ios")))]?
| @@ -0,0 +1,87 @@ | |||
| # Target OS | |||
There was a problem hiding this comment.
This is fantastic, thank you! I love the use of examples to call out the behavior in the more complicated scenarios 🎉
charlespierce
dismissed
seanaye’s stale review
Issue was a typo in a test case, which was resolved
--target-osvalues.#[cfg(not(..))]target_os parsing.printlneprintln.To run the test and see the evaluation:
RUST_LOG=debug cargo t --all-features --test snapshot_tests excluded_by_target_os::typescript -- --nocapture