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

Don't use booleans as function argument for function with 2+ arguments #964

Closed
Velfi opened this issue Dec 13, 2021 · 3 comments · Fixed by #1875
Closed

Don't use booleans as function argument for function with 2+ arguments #964

Velfi opened this issue Dec 13, 2021 · 3 comments · Fixed by #1875
Assignees
Labels
breaking-change This will require a breaking change enhancement New feature or request ergonomics good first issue Good for newcomers sdk-ga
Milestone

Comments

@Velfi
Copy link
Contributor

Velfi commented Dec 13, 2021

Instead, we should use enums with descriptive names:

const GREEDY: &AsciiSet = &LABEL_SET.remove(b'/');

pub enum EncodingStrategy {
  Default,
  Greedy,
}

pub fn fmt_string<T: AsRef<str>>(t: T, strategy: EncodingStrategy) -> String {
    let uri_set = if strategy == Greedy { GREEDY } else { LABEL_SET };
    percent_encoding::utf8_percent_encode(t.as_ref(), uri_set).to_string()
}

let _ = fmt_string("some str", EncodingStrategy::Greedy);
// used to look like
// let _ = fmt_string("some str", true);
@Velfi Velfi added enhancement New feature or request ergonomics labels Dec 13, 2021
@jdisanti jdisanti added the breaking-change This will require a breaking change label Sep 30, 2022
@jdisanti jdisanti added this to the SDK GA milestone Sep 30, 2022
@jdisanti jdisanti added the good first issue Good for newcomers label Sep 30, 2022
@ysaito1001 ysaito1001 self-assigned this Oct 17, 2022
@ysaito1001
Copy link
Contributor

I am a fan of replacing bools with enums in function parameters. As part of this breaking change, do we want to make urlencode::BASE_SET: &AsciiSet pub(crate) (I think it's referred to as LABEL_SET in the given code snippet above)? As far as I can see, that's the only item for which we expose AsciiSet from this crate, and if we can avoid exposing it, would that be better?

@jdisanti
Copy link
Collaborator

AsciiSet comes from the percent_encoding crate, so it would be best to not expose it in the public API. That way, if percent_encoding makes breaking changes in a new major version, we will be able to upgrade to that version without making breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change enhancement New feature or request ergonomics good first issue Good for newcomers sdk-ga
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants