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

Add to_string and to_string_alt, along with alloc and std features #36

Closed
wants to merge 1 commit into from

Conversation

programmerjake
Copy link
Member

Fixes #20

@alexcrichton
Copy link
Member

Thanks for the PR!

Some time has passed since I originally commented on the issue, but nowadays I think actually it might be best to avoid this. This looks like a good deal of complexity without much gain unfortunately, since .to_string() already exists as a method with libstd and I don't think there's much of a precedent for methods like .to_string_alt()

@programmerjake
Copy link
Member Author

programmerjake commented Jan 26, 2020

what do you think about instead extending the API to be something like this:

#[derive(Default, Clone, Debug)]
pub struct Config {
    with_hash: bool,
    // extend with more config options in the future
}

impl Config {
    pub fn new() -> Self {
        Self::default()
    }
    pub fn with_hash(self, with_hash: bool) -> Self {
        Self { with_hash, ..self }
    }
}

impl Demangle {
    // Demangle starts with Config::new() and can be changed with this fn;
    // Debug and Display impl's format with provided config,
    // but enable with_hash when `#` is used for backward-compat
    pub fn with_config(self, config: Config) -> Self {
        ...
    }
}

@alexcrichton
Copy link
Member

That's a possibility, but it still seems like somewhat overkill to me compared to a.to_string() or format!("{:#}", a)

@programmerjake
Copy link
Member Author

programmerjake commented Jan 27, 2020

That's a possibility, but it still seems like somewhat overkill to me compared to a.to_string() or format!("{:#}", a)

it may be overkill for now, but it is not overkill for being a good way to allow adding other display options in the future.

Admittedly, this should be a new PR.

@alexcrichton
Copy link
Member

Yeah that's sort of how I feel. What's here isn't exactly the most extensible design in the world in terms of having an obvious avenue to support all sorts of future features, but that being said there aren't any future features that I know of.

@programmerjake programmerjake deleted the master branch January 30, 2020 03:22
@programmerjake
Copy link
Member Author

#31 seems like a good candidate for a future feature that could benefit from a more-extensible config.

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.

Allow taking an optional dependency on std to directly output to a String
2 participants