Remove the matchers crate #2945
Conversation
wyatt-herkamp
requested review from
hawkw,
davidbarsky and
a team
as code owners
|
Funny story. The file was corrupted on my system so that bug I was seeing was a corrupted local cargo registry. However, I still think it is a good idea to remove the matchers crate as it seems to be no longer maintained. |
There was a problem hiding this comment.
The matchers code feels a bit out of places in an ideal world we could just update the code there.
There seems to still be at least some development there, the last comment about the update to a more recent reflex automata version is from March.
There also seem to be some considerations about the upgrade which could potentially break existing usage, are you aware of those and did you take that into account?
Personally, I'd give it a bit more time before moving the code inside this project.
| @@ -279,9 +279,9 @@ impl fmt::Display for ValueMatch { | |||
| // === impl MatchPattern === | |||
|
|
|||
| impl FromStr for MatchPattern { | |||
| type Err = matchers::Error; | |||
| type Err = regex_automata::dfa::dense::BuildError; | |||
There was a problem hiding this comment.
You have the type imported so there is no need to fully name it.
.gitignore
Outdated
| @@ -1,4 +1,3 @@ | |||
|
|
|||
There was a problem hiding this comment.
I don't think any of these changes should be in this PR. Whether it should be ignored or not is for a different discussion.
tracing-subscriber/Cargo.toml
Outdated
| @@ -42,7 +42,7 @@ tracing-core = { path = "../tracing-core", version = "0.2", default-features = f | |||
|
|
|||
| # only required by the `env-filter` feature | |||
| tracing = { optional = true, path = "../tracing", version = "0.2", default-features = false } | |||
| matchers = { optional = true, version = "0.1.0" } | |||
| regex-automata = { optional = true, version = "0.4.6",default-features = false, features = ["syntax", "dfa-build", "dfa-search"] } | |||
There was a problem hiding this comment.
Nit, you're missing a space after the comma after version.
I did see people are trying to update the crate. However, the developer doesn't seem to be making it a high priority. With it only being about 100 lines of code to implement I don't think using the matchers crate is providing a huge benefit here.
I understand. However, we are not using all the features that the matchers crate provides. And shrinking the dependency tree can be a good thing as it can lead to better compile times and less likely to get attacked via a supply chain attack. |
|
matchers 0.2 with updated dependency is released now. It would be great if this issue could be resolved one or the other way. |
|
Should I go ahead and close this PR then? |
|
I opened #3033 which updates matchers to 0.2. Let's see which is merged, if any, |
|
I am closing this PR. I hope that eventually this crate will be updated. |
Motivation
The matcher's crate has not been updated in 3 years. It is on Regex Automata version "0.1". Due to this I am getting a weird utf-8 error. This part is not a tracing problem. Just an outdated library issue.
Solution
This removes the matchers crate. The code is inspired from matchers crate with updates to Regex Automata 0.4.6. This also strips down the code as we don't need all the features matchers provided.
All tests were passed and has zero user facing changes.
Also this adds .vscode/settings.json to the .gitignore as that file can be developer settings. For instance I needed to enable all features for rust-analyzer.