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

feat(fmt): support fmt::Writers when writing ini to a Writer #131

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iiian
Copy link

@iiian iiian commented May 1, 2024

Allows for

ini.to_string()
ini.write_to_fmt(&mut std::fmt::Writer)
ini.write_to_policy_fmt(&mut std::fmt::Writer, EscapePolicy)
ini.write_to_opt_fmt(&mut std::fmt::Write, WriteOption)

Context

I'm working with a very laggy remote Windows file system. Each individual R/W request has about 2 seconds delay before actually getting serviced. This means the streaming write!-based implementation of write_to_opt() is resulting in something like an O(n) slow-down where n is the # lines in the file. Introducing a fmt::Writer version, write_to_opt_fmt, lets me buffer the entire ini as a String and then fire it in one shot, resulting in a 75x performance improvement in my test scenario with an INI 462 lines long.

ini.with_section(Some("foo"))
.add("a", "1")
.add("a", "2");
ini.with_section(Some("foo")).add("a", "1").add("a", "2");
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargofmt strikes

@@ -959,6 +969,59 @@ impl Ini {
}
}

impl Ini {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this impl is a verbatim scrape of the impl Ini block that defines the likes of write_to, write_to_policy, write_to_opt, but replaces each with std::fmt::Writer instead of std::io::Writer

}
writer
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly this is negotiable, it could be nice to have but I'm perfectly fine cutting it out.

@iiian iiian changed the title feat(ToString): implement ToString feat(fmt): support fmt::Writers when writing ini to a Writer May 1, 2024
@zonyitoo
Copy link
Owner

zonyitoo commented May 2, 2024

Please resolve the windows CI build failure (\r\n issue).

replace with a CRLF fix
@zonyitoo
Copy link
Owner

zonyitoo commented May 5, 2024

Still broken.

@iiian
Copy link
Author

iiian commented May 8, 2024

Hrm... I'm just not sure why this isn't working. I loaded rust-ini up on my Windows 11 machine, with supporting VisualC++ tooling -- all the unit tests are still passing. It seems to boil down to my inability to get the rust runtime to treat

r"
hello
world
"

as having CRLFs, even though there are unit tests above mine that seemingly do the exact same thing.

Any ideas?

@zonyitoo
Copy link
Owner

You may consider add an option to the write_to_fmt call, which will force line-break to use \n.

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.

2 participants