Skip to content
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

remove regex dependency #1817

Merged
merged 1 commit into from Dec 23, 2020
Merged

remove regex dependency #1817

merged 1 commit into from Dec 23, 2020

Conversation

danburkert
Copy link
Contributor

regex showed up in the cargo bloat profile of a large application I work on, and rusoto is the only crate which depends on it. It appears that rustoto's use of it is very light, so this PR removes the regex dependency and replaces it with some simple string splitting logic.

No tests are included, but I believe this functionality is already tested by the sample configs in the test suite.

Please help keep the CHANGELOG up to date by providing a one sentence summary of your change:

  • Remove regex dependency.

@rcoh
Copy link

rcoh commented Sep 7, 2020

I don't think this is quite right: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f5d78ab8f37b6f4fa60dc5f851c8b883

It seems to only return the first character of the user. This seems to do the trick:

fn parse_profile_name(line: &str) -> Option<&str> {
    let line = line.strip_prefix("[").and_then(|line|line.strip_suffix("]"))?;
    let line = line.strip_prefix("profile").unwrap_or(line);
    Some(line.trim())
}

however, I'd probably add some tests for edge cases.

@softprops
Copy link
Contributor

Rusoto currently uses the regex crate's default features.

Have you explored disabling those? See rust-lang/regex#613 for an example of how to shrink the binary size is if binary size is the concern

The trade of I see here is one of complexity added to rustoto when dropping the dependency all together.

Regex is fairly common dependency in other crates and applications. The complexity might not be worth it in those cases.

@danburkert danburkert force-pushed the remove-regex branch 2 times, most recently from b8b3db6 to 6084556 Compare September 8, 2020 05:14
@danburkert
Copy link
Contributor Author

danburkert commented Sep 8, 2020

I don't think this is quite right: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f5d78ab8f37b6f4fa60dc5f851c8b883

Yep, and actually a bunch (all?) of the tests failed, I just didn't run them on my final version 😬 . Fixed now.

Rusoto currently uses the regex crate's default features. Have you explored disabling those?

I did not consider it, because the usage of regex is very minimal.

The trade of I see here is one of complexity added to rustoto when dropping the dependency all together.

Could you expand on this thought? I don't see how dropping a dependency could inherently add complexity. If this PR is merged it does not preclude regex from being added as a dependency in the future if new functionality needs it.

Regex is fairly common dependency in other crates and applications.

My motivating scenario is an application which has ~100 direct dependencies with ~500 transitive dependencies, and this was the only direct dependency which depends on regex.

@riquito
Copy link

riquito commented Sep 8, 2020

I don't see how dropping a dependency could inherently add complexity

That's because you shift the burden from the dependency to your codebase, which usually gains more code to maintain.
But that's a general rule, here the increase in lines of code is almost none, and you remove a dependency. Imho it's a win.
It's true that regex is a common crate, but a single simple usage could be not worth the bloat.

The main function could be perhaps simplified as

/// Parses a profile header, returning the profile name.
fn parse_profile_name(line: &str) -> Option<&str> {
    let line = line.trim();
    if line.starts_with("[") && line.ends_with("]") {
        let profile_name = &line[1..line.len() - 1];
        Some(profile_name.trim_start_matches("profile "))
    } else {
        None
    }
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=3edeb7d20057e5a79d1d78a356cb5bf5

Strings are utf-8 encoded and you're guaranteed that those two characters are 1 byte long (please correct me if I'm wrong). It avoids the repetition of "profile " which could lead to typos in one of them. It reduces wrongly [profile profile x] to x which I don't know if can be a thing though

@danburkert
Copy link
Contributor Author

danburkert commented Sep 8, 2020

But that's a general rule, here the increase in lines of code is almost none, and you remove a dependency. Imho it's a win.

Great, I think we're in agreement.

Strings are utf-8 encoded and you're guaranteed that those two characters are 1 byte long (please correct me if I'm wrong).

Yep all the characters here are guaranteed to be 1 byte, but string slicing is fraught enough that I prefer to provide offsets as "[".len() instead of just 1, because it's much more obvious that you've 'done it right', and it's a bit more robust to future changes. It does require repeating the literal strings though, since you must do a match check before the index operation.

It avoids the repetition of "profile " which could lead to typos in one of them. It reduces wrongly [profile profile x] to x which I don't know if can be a thing though

I agree, strip_prefix would be perfect, but it requires rust 1.45. I looked but couldn't find an MSRV for rusoto, so I opted to be conservative. Happy to switch to strip_prefix if the maintainers prefer, though.

@iliana
Copy link
Contributor

iliana commented Sep 8, 2020

I am generally fine with very recent MSRVs. (We moved to std::future::Future a month or so after it landed in stable.)

The dependency on `regex` showed up in the `cargo bloat` profile of a
large application I work on, and `rusoto` is the only crate which
depends on it. It appears that `rustoto`'s use of it is very light, so
this PR removes the regex dependency and replaces it with some simple
string splitting logic.
@benesch benesch merged commit 553d1ed into rusoto:master Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants