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

Experimental parse-display POC #785

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Feb 11, 2024

It may be possible to eliminate a lot of fmt::Display code by using parse-display crate. In the first iteration, it can replace many display traits for the simple cases of constants and formatted inner values. In the more advanced, it should be possible to format iterators, and if the issue I proposed get implemented, might even cover many of the fmt_sse functions.

Note that I made a few unit tests for the migration purposes - just to see that the result is identical. We may want to remove at least some of them later on as being too trivial.

One aspect that may need discussion:

write!(f, "{value}") is not the same as value.fmt(f) because the first case creates a new Formatter instance, whereas the second case reuses the one passed as an argument to Display::fmt function.

In some cases, it may break if formatter contains padding or number formatting configuration that will or won't be passed to the nested object. parse-display seem to always generate a new Formatter, but Oxigraph uses a lot of .fmt calls - which might actually be a bug.

It may be possible to eliminate a lot of `fmt::Display` code by using [parse-display](https://github.com/frozenlib/parse-display/tree/master?tab=readme-ov-file#parse-display) crate.  In the first iteration, it can replace many display traits for the simple cases of constants and formatted inner values.  In the more advanced, it should be possible to format iterators, and if the issue I proposed get implemented, might even cover many of the `fmt_sse` functions.

Note that I made a few unit tests for the migration purposes - just to see that the result is identical. We may want to remove at least some of them later on as being too trivial.

One aspect that may need discussion:

`write!(f, "{value}")` is not the same as `value.fmt(f)` because the first case creates a new `Formatter` instance, whereas the second case reuses the one passed as an argument to `Display::fmt` function.

In some cases, it may break if formatter contains padding or number formatting configuration that will or won't be passed to the nested object.  `parse-display` seem to always generate a new Formatter, but Oxigraph uses a lot of `.fmt` calls - which might actually be a bug.
@Tpt
Copy link
Collaborator

Tpt commented Feb 11, 2024

Hi! Thank you! I am often a bit not at ease with adding macro driven code. I like to keep things simple and a big match seems to me easier to understand and tweak than complex macros and extra dependencies.

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

2 participants