Configurable autoformat with FormatConfig
#95
Conversation
…`KdlNode::autoformat_config`
| pub indent_level: usize, | ||
|
|
||
| /// The indentation to use at each level. Defaults to four spaces. | ||
| pub indent: &'a str, |
There was a problem hiding this comment.
I went with just indent for the field name here, and a &str for repetitions of the same character.
There was a problem hiding this comment.
Alternatively, this could be an enum of Tab and Spaces(usize) to constrain it to valid options 🤔
Cargo.toml
Outdated
| @@ -16,6 +16,7 @@ default = ["span"] | |||
| span = [] | |||
|
|
|||
| [dependencies] | |||
| bon = { version = "2.1.0", default-features = false } # 3.2.0 requires rustc 1.59.0 | |||
There was a problem hiding this comment.
This pulls in quite a few dependencies for the procedural macro.
Unfortunately, making it optional would turn the code in fmt quite messy.
There was a problem hiding this comment.
oh, if it pulls in a lot, maybe it's better to hand-write the builder. It's really not a lot of trouble, even if it feels like a bit of boilerplate, imo.
There was a problem hiding this comment.
or actually, if the config is defaultable, just don't bother with the builder at all, tbh.
There was a problem hiding this comment.
Will do 👍
Yes, it pulls in Syn. (I think it would be helpful to commit Cargo.lock to the repository, in part to see those changes in PRs.)
There was a problem hiding this comment.
or actually, if the config is defaultable, just don't bother with the builder at all, tbh.
Oh, I missed this comment. Should I remove the builder?
I think it might be convenient to have the defaults const-available though.
|
I allowed the caller to control outer indentation here (which may be useful for nodes. Is that expected or would you prefer if only the indentation per level can be chosen? …actually yeah I misunderstood how this works, I'll fix it. |
|
I misunderstood how I misunderstood 😅 I could do an alternative approach where |
|
This looks really good tbh. Like I said in a separate comment, I honestly don't think we need a builder at all for this. Bon definitely does not seem worth it for this tiny config struct, either. Also: how would you feel about addressing #64 while you're at it? It can be done in a separate PR by whoever if you want to keep this one more focused, though. |
Fortunately really not too bad here, since every field is defaulted. This is less validating than what Bon generates though, since now there are no safeguards against repeating settings. On the plus side, it's completely `const`.
|
I replaced the builder implementation with a manual one. It now doesn't validate that the fields don't repeat (because that would be messy enough to warrant the dependency), but on the plus side it's all-
It would be more complicated since it definitely should auto-detect if you add options there. I don't think I'll implement that 😅 |
This implements the discussion in #85, i.e.
(KdlDocument|KdlNode)::autoformat_config(&mut self, config: &FormatConfig)(replacing the respective internalautoformat_implfunctions), with small changes to theFormatConfigfields.The first commit adds just the configurable formatting with tests, the second one adds a builder API to
FormatConfig.