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

easy::to_string_pretty doesn't produce pretty results #192

Closed
epage opened this issue Sep 10, 2021 · 8 comments · Fixed by #286
Closed

easy::to_string_pretty doesn't produce pretty results #192

epage opened this issue Sep 10, 2021 · 8 comments · Fixed by #286
Labels
C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.

Comments

@epage
Copy link
Member

epage commented Sep 10, 2021

No description provided.

@epage epage added the C-bug Category: Things not working as expected label Sep 10, 2021
@epage
Copy link
Member Author

epage commented Sep 10, 2021

I propose that Document, Table,, etc have fmt functions that take a format configuration. to_string_pretty and other serializer options are implemented in terms of this.

This function will be the public facing one. An internal one will be used when recursing so we can pass in the relevant information from the caller, in addition to the format config.

@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Sep 12, 2021
@heisen-li
Copy link

Hello, I've just touched this. Let me state my understanding of the question and see if my understanding is correct.

It seems that you don't think the printout of the current structure, such as Table and document, is perfect. Therefore, the function easy::to_string_pretty needs to be optimized, right?

If my understanding is correct, what is the perfect output format you want?
Please give me a detailed description. Thank you.

@epage
Copy link
Member Author

epage commented Sep 24, 2021

I'd say we start with mimicing toml-rs, ie able to pass their tests

We can look to their serde implementation for a starting point for the algorithm: https://github.com/alexcrichton/toml-rs/blob/master/src/ser.rs

Without looking to closely

  • We should always create valid toml
  • We should prefer array of tables over inline arrays
  • If a table has too many elements, we should prefer standard tables over inline tables.

@epage
Copy link
Member Author

epage commented Nov 16, 2021

Example failure from cargo's test suite

---- features_namespaced::publish stdout ----                                                                                                      
running `/home/epage/src/personal/cargo/target/debug/cargo publish --token sekrit -Z namespaced-features`                                          
thread 'features_namespaced::publish' panicked at '                      
                                                                                                                                                   
error:  did not match:                                                                                                                             
1   1     # THIS FILE IS AUTOMATICALLY GENERATED BY CARGO                                                                                          
2   2     #                                                                                                                                        
3   3     # When uploading crates to the registry Cargo will automatically                                                                         
4   4     # "normalize" Cargo.toml files for maximal compatibility                                                                                 
5   5     # with all versions of Cargo and also rewrite `path` dependencies                                                                        
6   6     # to registry (e.g., crates.io) dependencies.                                                                                            
7   7     #                                                                                                                                        
8   8     # If you are reading this file be aware that the original Cargo.toml                                                                     
9   9     # will likely look very different (and much more reasonable).                                                                            
10  10    # See Cargo.toml.orig for the original contents.                                                                                         
11  11                                                                                                                                             
12       -[package]                                                                                                                                
13       -name = "foo"                                                                                                                             
14       -version = "0.1.0"                                                                                                                        
15       -description = "foo"                                                                                                                      
16       -homepage = "https://example.com/"                              
17       -license = "MIT"                                                                                                                          
18       -[dependencies.bar]                                                                                                                       
19       -version = "1.0"                                                                                                                          
20       -optional = true                                                                                                                          
21       -                                                                                                                                         
22       -[features]                                                                                                                               
23       -feat1 = []                                                                                                                               
24       -feat2 = ["dep:bar"]                                                                                                                      
25       -feat3 = ["feat2"]                                                                                                                        
    12   +package = { name = "foo", version = "0.1.0", description = "foo", homepage = "https://example.com/", license = "MIT"}                    
    13   +dependencies = { bar = { version = "1.0", optional = true}}                                                                              
    14   +features = { feat1 = [], feat2 = ["dep:bar"], feat3 = ["feat2"]}                                                                         

@epage epage added the cargo label Nov 16, 2021
@sunshowers
Copy link
Contributor

Thanks! I ran across this while trying to see if I can switch my code to using toml_edit (which looks fantastic btw!)

I'm wondering if at some point, finer-grained control might be desirable -- perhaps some sort of trait that can control whether a particular table is serialized as inline or standard. e.g. for a Cargo.toml, I'd like to see dependencies.bar be an inline table but features be a standard table.

sunshowers added a commit to sunshowers/cargo-guppy that referenced this issue Nov 20, 2021
This means that the same library can parse summaries created by both old
and new versions of guppy. We don't use the summary for much anyway,
currently.

I also looked at switching to `toml_edit`, which has a much better
design overall. Unfortunately, I ran into
toml-rs/toml#192. It makes sense to switch
to `toml_edit` at some point in the future, and I've filed facebookarchive#492 to keep
track of that.
@epage
Copy link
Member Author

epage commented Nov 21, 2021

I'm wondering if at some point, finer-grained control might be desirable -- perhaps some sort of trait that can control whether a particular table is serialized as inline or standard. e.g. for a Cargo.toml, I'd like to see dependencies.bar be an inline table but features be a standard table.

That makes sense. A similar thought i had was a way to control sort order (e.g. [package], [features], [dependencies], [target]).

The main thing is balancing the complexity, whether thats in a user finding what they want and understanding how to use it, implementing the users code on top of the API, or the implementation within the library. At a certain point of proliferation of customization, it seems like it'd be better for someone to walk the data structure and format it manually. I guess another aspect of all of this is finding the answer to what can we do to support people writing that custom formatting.

btw guppy looks interesting! In particular, I find the idea of determinator interesting from two angles

  • For build/test avoidance in open source. For example, clap's CI takes over an hour (they did switch to a two-tier CI using bors so that cost is only paid on merge and not each PR update, instead its only a half hour or so which is still bad). I would love to also have coverage-driven test case avoidance but thats its own separate thing.
  • As a maintainer of one of the release automation tools (cargo-release). We've all been implementing our own bespoke implementations of release-avoidance. These have their own needs. For a release, all you care about is included/excluded files. You also only need to release all packages with a [[bin]] if the lock file changed. I wish there was a way to know if a dependency showed up in a public API.

@sunshowers
Copy link
Contributor

That makes sense. A similar thought i had was a way to control sort order (e.g. [package], [features], [dependencies], [target]).

The main thing is balancing the complexity, whether thats in a user finding what they want and understanding how to use it, implementing the users code on top of the API, or the implementation within the library. At a certain point of proliferation of customization, it seems like it'd be better for someone to walk the data structure and format it manually. I guess another aspect of all of this is finding the answer to what can we do to support people writing that custom formatting.

Oh that's a super interesting idea. I wonder what an API based on syn::visit_mut would feel like. Going to try playing with it a bit over the next few days.

btw guppy looks interesting! In particular, I find the idea of determinator interesting from two angles

Thank you! Yeah, the determinator is really cool and has proven to work quite well in a large workspace. I think both of the use cases you described make a lot of sense.

I haven't seen much demand for it in open source yet but now that you've asked for it, I now have a reason to clean it up and put it out there :) Happy to chat about this in more detail on Discord or some other medium.

sunshowers added a commit to sunshowers/cargo-guppy that referenced this issue Nov 23, 2021
This means that the same library can parse summaries created by both old
and new versions of guppy. We don't use the summary for much anyway,
currently.

I also looked at switching to `toml_edit`, which has a much better
design overall. Unfortunately, I ran into
toml-rs/toml#192. It makes sense to switch
to `toml_edit` at some point in the future, and I've filed facebookarchive#492 to keep
track of that.
sunshowers added a commit to facebookarchive/cargo-guppy that referenced this issue Nov 23, 2021
This means that the same library can parse summaries created by both old
and new versions of guppy. We don't use the summary for much anyway,
currently.

I also looked at switching to `toml_edit`, which has a much better
design overall. Unfortunately, I ran into
toml-rs/toml#192. It makes sense to switch
to `toml_edit` at some point in the future, and I've filed #492 to keep
track of that.
epage added a commit to epage/toml_edit that referenced this issue Dec 31, 2021
This includes porting tests over from `toml-rs` to ensure we match their
behavior.  We removed the non-pretty case and the customization since
those aren't as important for our use case (cargo) and we'll want to
iterate on what we want from the API.

Our "pretty" is different than `toml-rs` in that we prefer `"` for
strings that could be literals but don't need to be literals to avoid
escaping.

Note: there are some other discrepancies in our "pretty" string handling
that should be looked into, like switching to tripple quotes to avoid
escaping.

Fixes toml-rs#192
epage added a commit to epage/toml_edit that referenced this issue Dec 31, 2021
This includes porting tests over from `toml-rs` to ensure we match their
behavior.  We removed the non-pretty case and the customization since
those aren't as important for our use case (cargo) and we'll want to
iterate on what we want from the API.

Our "pretty" is different than `toml-rs` in that we prefer `"` for
strings that could be literals but don't need to be literals to avoid
escaping.

Note: there are some other discrepancies in our "pretty" string handling
that should be looked into, like switching to tripple quotes to avoid
escaping.

Fixes toml-rs#192
epage added a commit that referenced this issue Dec 31, 2021
This includes porting tests over from `toml-rs` to ensure we match their
behavior.  We removed the non-pretty case and the customization since
those aren't as important for our use case (cargo) and we'll want to
iterate on what we want from the API.

Our "pretty" is different than `toml-rs` in that we prefer `"` for
strings that could be literals but don't need to be literals to avoid
escaping.

Note: there are some other discrepancies in our "pretty" string handling
that should be looked into, like switching to tripple quotes to avoid
escaping.

Fixes #192
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Things not working as expected M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants