Regex cleanup#306
Conversation
|
Damn thats one massive PR, will try to review this in the following days |
|
@MordechaiHadad - Yeah I sort of realized this after submitting it. Despite most of the files having been touched a lot of them are the result of formatting - If you'd like I can separate out some of the formatting context + rebase it and split that into a different PR, may help? Either way let me know if there's feedback or preference to split it up 😃 |
|
@MordechaiHadad - Let me know if there's anything else you'd like cleaned up or done before a merge, happy to do so or talk through changes etc. |
d395787 to
e138c7c
Compare
e138c7c to
d3474f6
Compare
Prior: -edition = "2021" -rust-version = "1.82" Rust edition 2024 cannot be used on 1.82 as it requires at least 1.85. the min. version has been bumped to accomodate the edition change. chore: dev profile for compilation speed
chore: `extern crate <>` is not needed anymore
split out the ProgressBar logic from the response parsing logic
docs: EnvVarProcessor docs
d3474f6 to
9dcf23c
Compare
9dcf23c to
57824d2
Compare
|
I've also rebased this so it's up-to-date with |
57824d2 to
a585939
Compare
a585939 to
1e0a681
Compare
|
damn so many commits, ill need to rereview this again in a few days, btw you can safely ignore the deepsource and codacy nonsense, i am just testing those, so you dont need to bother. |
Further review showed lingering Regex calls that could be moved to `consts.rs` for better organization and performance.
This change also cleans up test module nesting
|
I've been using jj/jujutsu over git for a bit now but have yet to do PR's with it for the most part. Didn't realize when it does its own internal rebasing it actually handles them all individually (Makes sense I guess as the hash technically changes from git's perspective). I'll look into how jj handles GitHub's PR threads as yeah, that's a bit of a mess. Genuinely didn't realize it would generate that much noise. |
|
I guess we done really? cant really think of anything to change it looks good. |
|
Perfect then 💯 I'm aware of the other PR(s) that potentially touch around regex (I've not looked at it yet, but I assume #315 probably does). |
|
merging this now, works both on linux and windows. dont own a mac to tests, but if both work i doubt mac will fail. |
Performance and Code Quality Improvements: Regex Optimization and Refactoring
Summary
This PR introduces significant performance improvements and code quality enhancements through regex compilation optimization, code de-duplication, and some minor structural refactoring.
The changes move repeatedly compiled regex patterns to compile-time constants and implement several architectural improvements.
Aimed to keep the revset fairly small to make it easy to review, let me know if it's a bit much much.
The Short & Sweet / tl;dr / lgtm - Technical Improvements
LazyLock<Regex>patternsEnvVarProcessortrait for cleaner environment variable handlingKey Changes
Perf. Optimizations
Regex compilation: Moved all regex patterns from runtime compilation to compile-time static constants using the std lib's
LazyLock.The docs for the std lib's Regex mention that regex compilation is an expensive operation, and compiling regex patterns at runtime run every time they're read in.
This can be especially rough on performance in hot-loops. (I've always treated it as a case of 'if you can move it to compile time, do it'. (Such is Rusts philosophy anyway.))
This will also remove any potential overhead
bobmay have been encountering when making repeat calls to the functions.New constants module: Created 'src/consts.rs' with centralized regex patterns (
VERSION_REGEX,NIGHTLY_REGEX,ENVIRONMENT_VAR_REGEX)Moving Regex to compile time is of course the primary reason for the PR; however, centralizing also makes it easier to maintain & write tests for in the future.
Platform specific constants: Introduced
FILETYPE_EXTconstants for platform-specific file extensions ('tar.gz' vs. 'zip')This was previously a function call with an internal
cfg!(target_family = _ )macro call.Given the function call only exists to make an if-else decision based on a compile-time flag set by
cargo, it makes sense to move this to be just a straight-up constant instead.Conditional compilation can still be expanded via 'target_os' vs 'target_family' etc. or the function re-implemented again later if needed.
Code Structure & Quality
Env. Variable processing: Refactored config.rs to use a polymorphic trait-based approach, favoring array->trait(), which is more idiomatic in Rust and should also make it easier
to extend in the future if
bobdecided to further support Environment Variables without moving towards something like theconfigcrate.install_handler.rscleanup: Minor streamlining for readability, extracting the progress bar logic into PbWrapper struct, very minor code organization done in src/handlers/install_handler.rsNote: Had I more time, I would have liked to have done a more thorough refactor of the
install_handler.rsfile (ie: cleaning up boolean flags & preferring type-heavier pattern-matching).That being said this PR was mostly to resolve the Regex, so I didn't want to go too far off the beaten path.
Import path standardization: Consistently prefer
crate::*oversuper::*import calls throughout the codebase.super::<>as a preference becomes a lot harder to manage if a project grows in size, with refactoring becoming a pain point due to imports when moving files around.Note: I'd like to highlight that these specific changes did involve modifications to the
#[cfg(test)]module items insrc/helpers/mod.rs- by way of removal of the prefixedsuper::and moving that to the module level.I've run tests locally (via Arch-Linux) and they all pass and I've used the binary to do installs/uninstalls etc.
Performance Impact