Conversation
mhils
force-pushed
the
no-default-features
branch
from
a6c7961 to
733a049
Compare
| <flakyError type="flaky error type">flaky error description | ||
| <stackTrace>flaky stack trace</stackTrace> | ||
| <system-out>flaky system output</system-out> | ||
| <system-err>flaky system error with [34mANSI escape codes[39m</system-err> |
There was a problem hiding this comment.
Without the ansi-escape feature flag, ANSI escape codes are just added to the XML. It looks like there is actually no additional escaping necessary to be valid XML (but if it were, quick-xml would take care of it).
Of course we shouldn't have unescaped ANSI codes in here, but in many cases you may know that the target application is never producing them in the first place.
There was a problem hiding this comment.
Interesting -- I think a newer version of quick-xml might be doing some filtering here.
sunshowers
left a comment
sunshowers
left a comment
There was a problem hiding this comment.
Thanks for working on this!
| [features] | ||
| default = ["timestamps", "uuids", "strip-ansi"] | ||
| timestamps = ["dep:chrono"] | ||
| uuids = ["dep:uuid", "dep:newtype-uuid"] |
There was a problem hiding this comment.
Let's call this feature uuid.
| rust-version = "1.70" | ||
|
|
||
| [features] | ||
| default = ["timestamps", "uuids", "strip-ansi"] |
There was a problem hiding this comment.
I'd make timestamps default and the other two non-default.
| #[cfg(feature = "strip-ansi")] | ||
| let data = strip_ansi_escapes::strip_str(data); |
There was a problem hiding this comment.
This isn't right -- enabling a feature shouldn't cause this kind of behavior change. Instead I'd recommend adding an alternative constructor.
There was a problem hiding this comment.
To clarify, you mean something like XmlString::escape_ansi() -> Self? XmlString::new would not do automatic stripping and consumers would use this alternative constructor if they need to strip ANSI escapes?
There was a problem hiding this comment.
For backwards compatibility, I think new should retain the strip-ansi behavior, and there should be a separate constructor that only removes bytes illegal in XML without stripping ANSI codes. The latter could be called new_minimal or something. (This also suggests that the strip-ansi feature should be turned on by default.)
Oh it looks like in the loop below, it's quick-junit that's doing the filtering of illegal bytes.
| <flakyError type="flaky error type">flaky error description | ||
| <stackTrace>flaky stack trace</stackTrace> | ||
| <system-out>flaky system output</system-out> | ||
| <system-err>flaky system error with [34mANSI escape codes[39m</system-err> |
There was a problem hiding this comment.
Interesting -- I think a newer version of quick-xml might be doing some filtering here.
2 participants
This PR fixes #429. I've moved uuid, chrono and strip-ansi-escapes into optional (on-by-default) features, and then made sure this configuration is tested in CI as well.
Thank you for building and maintaining quick-junit! 🍰