Improve Metadata initialization time, add benchmarking, check regexps during build & general housekeeping #27
Conversation
| @@ -126,6 +126,9 @@ pub enum LoadMetadata { | |||
|
|
|||
| /// Malformed Regex in Metadata XML database | |||
| #[error("Malformed Regex: {0}")] | |||
| Regex(#[from] regex::Error), | |||
| Regex(#[from] regex::Error), | |||
There was a problem hiding this comment.
Something funky going on here with spaces/tabs.
There was a problem hiding this comment.
that was weird, fixed
yannleretaille
force-pushed
the
master
branch
from
ab0c8f3 to
66759a1
Compare
|
changed to semantic commit messages |
…g Metadata from the Database This has two benefits: - it results in a ~97% reduction of initiliaztion time the first time a phone number is validated. This is crucial for client side applications. - it prevents the crate from compiling in the unlikely case that Googles Metadata should ever contain invalid regexps (this was tested) In the future it should be investigated if there is other data that could be pre-validated during the build phase fixes whisperfish#26
yannleretaille
force-pushed
the
master
branch
from
66759a1 to
4f8cfde
Compare
| @@ -70,7 +71,7 @@ pub use crate::carrier::Carrier; | |||
| mod phone_number; | |||
| pub use crate::phone_number::{PhoneNumber, Type}; | |||
|
|
|||
| mod parser; | |||
| pub mod parser; | |||
There was a problem hiding this comment.
I guess you made this public because of the benchmark.
But it's not supposed to be part of the public API so it probably should be #[doc(hidden] or similar.
| } | ||
|
|
||
| /// Create a database from a loaded database. | ||
| pub fn from(meta: Vec<loader::Metadata>) -> Result<Self, error::LoadMetadata> { | ||
| pub fn from(meta: Vec<loader::Metadata>, check_regex: bool) -> Result<Self, error::LoadMetadata> { |
There was a problem hiding this comment.
This is a braking change.
Furthermore the api between from, load and parse is no inconsistent.
If we anyway do a braking change we probably want to have some opaque Options type we can pass into all of from, load and parse.
|
Good points, I'll see what I can do!
…On Sun, Oct 18, 2020, 19:34 Philipp Korber ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I could merge it the way it is and do the noted changes later before
publishing a new version. ***@***.*** <https://github.com/meh>)
------------------------------
In src/lib.rs
<#27 (comment)>
:
> @@ -70,7 +71,7 @@ pub use crate::carrier::Carrier;
mod phone_number;
pub use crate::phone_number::{PhoneNumber, Type};
-mod parser;
+pub mod parser;
I guess you made this public because of the benchmark.
But it's not supposed to be part of the public API so it probably should
be #[doc(hidden] or similar.
------------------------------
In src/metadata/database.rs
<#27 (comment)>
:
> }
/// Create a database from a loaded database.
- pub fn from(meta: Vec<loader::Metadata>) -> Result<Self, error::LoadMetadata> {
+ pub fn from(meta: Vec<loader::Metadata>, check_regex: bool) -> Result<Self, error::LoadMetadata> {
This is a braking change.
Furthermore the api between from, load and parse is no inconsistent.
If we anyway do a braking change we probably want to have some *opaque*
Options type we can pass into all of from, load and parse.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#27 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAF3W33JHMVRR35BQ3XVLUTSLMRLJANCNFSM4SSDDMLQ>
.
|
|
@yannleretaille do you think you will have time to resume work on that PR? |
|
Any updates on this pr? |
|
I'm closing this as I can't hide it in my Git overview, and it likely won't get any process, and has been superseeded by the fork AFIKT. |
|
@rustonaut, we don't maintain a fork, you are writing in the live repository. The easiest way to get it out of your dashboard is probably to reduce your access rights in the repository (if you don't intend do exercise those rights any more), or to unassign issues and PRs. We initially wanted to keep this open, but @gferon will open an issue to track the follow-up here. |
|
Oh, sorry I missed that this is now a PR on |
Please note that these changes depend on the not yet released version 0.2.1 of regex-cache. This should fix #26
@meh @rustonaut would be great if you guys could have a look!