-
Notifications
You must be signed in to change notification settings - Fork 143
feat: Adding support for no_std environments for env_filter
#378
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
base: main
Are you sure you want to change the base?
Conversation
| #[cfg(not(feature = "std"))] | ||
| impl core::error::Error for ParseError {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the lack of std require a higher MSRV
| pub(crate) fn new(spec: &str) -> Result<Self, String> { | ||
| Ok(Self { | ||
| inner: spec.to_string(), | ||
| }) | ||
| } | ||
|
|
||
| pub fn is_match(&self, s: &str) -> bool { | ||
| pub(crate) fn is_match(&self, s: &str) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you split this out into a separate commit?
| use alloc::string::{String, ToString}; | ||
|
|
||
| use core::fmt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you group these?
| use alloc::borrow::ToOwned; | ||
| use alloc::format; | ||
| use alloc::string::String; | ||
| use alloc::vec::Vec; | ||
|
|
||
| use core::fmt::{Display, Formatter}; | ||
| use log::LevelFilter; | ||
| use std::error::Error; | ||
| use std::fmt::{Display, Formatter}; | ||
|
|
||
| use crate::Directive; | ||
| use crate::FilterOp; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're changing grouping, could you do
use {core,alloc,std}
use third-party
use crate
| use alloc::borrow::ToOwned; | ||
| use alloc::string::ToString; | ||
| use alloc::vec::Vec; | ||
|
|
||
| use core::fmt; | ||
| use core::mem; | ||
|
|
||
| use log::{LevelFilter, Metadata, Record}; | ||
|
|
||
| use crate::enabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| log = { version = "0.4.8", default-features = false } | ||
| regex = { version = "1.0.3", optional = true, default-features = false, features = ["std", "perf"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this belongs in the previous comit, could you move it there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor(filter): Add support for no_std environments
Mind fixing your commit type?
A refactor, by definition, has no user facing changes while this does. I would use feat
Pull Request Test Coverage Report for Build 17143312963Details
💛 - Coveralls |
Using
env_loggerin ano_stdenvironment may not seem useful at first. However, theenv_filtercrate is very convenient for defining complex filters that can then be implemented by a customno_std-compatible logger.This approach leverages the convenience developers are already familiar with from
env_logger, while enabling the same flexibility inno_stdcontexts with other logging backends.