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

Allow use of CodeFormatter from no_std #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thomcc
Copy link

@thomcc thomcc commented Aug 4, 2021

As a result, this removes the need for a feature = "std", and makes all the code in this crate unconditionally no_std (and without even needing extern crate alloc either).

It does this by making CodeFormatter generic over the type of the indentation, S: AsRef<str>. It's a breaking change, since now CodeFormatter has another generic parameter.

The generic parameter could be more general if instead it allowed S: fmt::Display rather than S: AsRef<str>. While that actually appeals to me a bit, I think it would not actually be very useful in practice, and it has some performance downsides1 that which run contrary to (if nothing else) the documented goals of CodeFormatter (which includes being efficient).

A downside of this is that now the type of the indentation string is in the type signature. This arguably is an encapsulation violation (but who cares), and more problematically, it means that if code want to pass a CodeFormatter<'_, T, &'static str> to a function expecting a CodeFormatter<'_, T, String> in the signature, it can't. I don't think this downside actually matters3, but felt like mentioning it because maybe you do.

This change invalidated part of the CodeFormatter docs, but I didn't know what to rewrite that part to, so I just deleted it. LMK if you have something you thing should go there instead.


(Sorry, somehow I wrote several footnote5s for this simple-seeming patch)

1: It's highly likely (if not guaranteed) that fmt::Write::write_str is going to be faster than invoking write!, but it also prevents this crate from doing various optimizations which are plausibly desirable. For example, this crate might want want to optimize repeated indents of all-" " strings2.

2: Note: that patch isn't a good way of doing this in retrospect — I realized a much simpler implementation is possible after writing it — but it's an example of the kind of thing that might be desirable, but requires S: AsRef<str>, and not S: Display (at least without access to specialization, unless we make various assumptions that likely defeat the point of using S: Display).

3: So, mostly I feel this way because its the same as the current situation for T. Also, I expect any usage that cares about the flexibility will just make their function generic over S. This is actually the path of least resistance, given we don't provide a default for S.

That said, I tried to think of ways to alleviate it, since it's certainly plausible someone could hit the issue. I don't think there's a great solution, but the big case I'd be concerned about is allowing code to pass a "child" CodeFormatter to a function that wants to write to it.

There is sort of a solution I came up with to this problem though. Probably anyway... the code below at least compiles4, and lets you get a CodeFormatter<'_, T, &str> out of a &mut CodeFormatter<'_, T, S> (the source must be &mut to hand the &mut T off to the child.

// Honestly, this is pretty odd to stuff this in a `From`,
// but it saves me from coming up with a name for this.
impl<'a, 'b, T, S> From<&'b mut CodeFormatter<'a, T, S>> for CodeFormatter<'b, T, &'b str>
where
    S: AsRef<str>,
    T: fmt::Write,
{
    fn from(cf: &'b mut CodeFormatter<'a, T, S>) -> CodeFormatter<'b, T, &'b str> {
        CodeFormatter {
            level: cf.level,
            f: &mut *cf.f,
            indentation: cf.indentation.as_ref(),
        }
    }
}

So, as you can see, I didn't include this. Mostly since IDK why the code that wants this wouldn't just be generic over S. Also, I'm not sure I can actually really explain what this does succinctly (it type-erases S into a &str mainly, but... also there's some wonkiness around the fact that it produces what seems like a copy, although writes still go to the same place, but level changes might not work as intended... etc).

That said, if you want me to include it, I'm totally happy to.

4: I haven't checked if it can actually be used — I'm not sure if the 'b lifetime being the same in both places would forbid that... I think it's fine though? I think you'd use it like &mut s.into(). Maybe this wouldn't infer right though, in which case an explicit name would be needed (no idea what a good name for it would be...)

5: They're... substantially longer than the non-footnote text now, and have their own sub-footnotes... I may have a problem... Sorry...

@yaahc
Copy link
Collaborator

yaahc commented Aug 4, 2021

cc @cecton who contributed the CodeFormatter

Also, @thomcc, loved the footnotes, loooool.

This all looks good to me but I want to double check with Cecile before merging. Also it looks like clippy failed.

@cecton
Copy link
Contributor

cecton commented Aug 5, 2021

I think I'm just gonna test ^_^' I have a huge private repo that uses this (with std).

@thomcc I suppose you tested in a nostd project and it works nicely?

@cecton
Copy link
Contributor

cecton commented Aug 5, 2021

🤔 hmm... there seems to be an issue.

Before:

#[derive(Debug, Clone, Copy, PartialEq, Hash, Serialize, Deserialize)]
pub enum Region {
    /// Austria
    At,
    /// Australia & NZ
    Au,
    /// Belgium
    Be,
    /// Central Eastern Europe
    Cee,
    /// Switzerland
    Ch,
    /// China
    Cn,
    /// Germany
    De,

After:

#[derive(Debug, Clone, Copy, PartialEq, Hash, Serialize, Deserialize)]
pub enum Region {
    ///
Austria
At,
                    ///
Australia & NZ
Au,
                    ///
Belgium
Be,
                    ///
Central Eastern Europe
Cee,
                    ///
Switzerland
Ch,
                    ///
China
Cn,
                    ///
Germany
De,

We will need to improve the tests somehow to reveal what is wrong.

Here is the relevant part of the code:

        write!(
            f,
            r#"
            #[derive(Debug, Clone, Copy, PartialEq, Hash, Serialize, Deserialize)]
            pub enum Region {{
            "#,
        )?;

        f.indent(1);
        for (_id, region) in self.regions.iter() {
            write!(
                f,
                r#"
                /// {}
                {},
                "#,
                region.label,
                region.name.to_camel_case(),
            )?;
        }

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.

3 participants