[pbs-devel] [PATCH proxmox-backup 3/5] fix #3067: api: add multi-line comments to node.cfg

[Stefan Sterz]

responses inline

On 23.02.22 11:28, Wolfgang Bumiller wrote:
> some comments inline
> 
> On Tue, Feb 22, 2022 at 12:25:54PM +0100, Stefan Sterz wrote:
>> -- snip --
>>   
>> +    pub MULTI_LINE_COMMENT_REGEX = r"(?m)^([[:^cntrl:]]*)\s*$";
> 
> I don't think the trailing `\s*` is necessary?
> 

Yes, you are right, stole that from the Perl implementation, which uses 
"m/^\#(.*)\s*$/" (to parse the configuration file, hence the extra 
"\#"). I am not too familiar with Perl so that's probably my mistake, 
but just to be sure, is it necessary over there [1]? Tried playing 
around with it a bit, but couldn't figure out the purpose of "\s" since 
"." already matches any character. Unless Perl does "lazy" matching 
here, then it trims the trailing whitespace. Note that, AFAICT, both "." 
and "[:^cntrl:]" do not match the line feed character (\n) (and 
[:^cntrl:] does not match the carriage return either), but that those 
occur after the "end of line" ("$") and, thus, would not be matched by 
"\s" either.

[1]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2369

>> -- snip -- 
>>  
>>   use pbs_api_types::{EMAIL_SCHEMA, OPENSSL_CIPHERS_TLS_1_2_SCHEMA, OPENSSL_CIPHERS_TLS_1_3_SCHEMA,
>> -    SINGLE_LINE_COMMENT_SCHEMA};
>> +    MULTI_LINE_COMMENT_SCHEMA};
> 
> Please check how rustfmt would deal with the above `use` statement ;-)
> 

Done, although rustfmt complains about quite a few more things in those 
three files. Should I leave them or let rustfmt fix them while I am at it?

>> -- snip -- >>>> +    let comment = input.lines()
>> +        .take_while(|l| l.starts_with('#'))
>> +        .map(|l| {
>> +            let mut ch = l.chars();
>> +            ch.next();
>> +            ch.as_str()
> 
> ^ The `take_while` ensures `l` starts with '#', so you could just use
> 
>      .map(|l| &l[1..])
> 
> Alternatively, since 1.57 (and we're at 1.58 now), you could also
> combine the `.take_while` and `.map` into:
> 
>      .map_while(|l| l.strip_prefix("#"))
> 
> However...
> 
>> +        })
>> +        .fold(String::new(), |acc, l| acc + l + "\n");
>> +
>> +    if !comment.is_empty() {
>> +        config.insert("comment".to_string(), Value::String(comment));
>> +    }
>> +
>>       for (lineno, line) in input.lines().enumerate() {
> 
> ... here we're starting over, so maybe we should refactor this a little.
> Eg. we could use a `.lines().enumerate().peekable()` iterator:
> 
>      let mut lines = input.lines().enumerate().peekable();
>      let mut comments = String::new();
>      while let Some((_, line)) = iter.next_if(|(_, line)| line.starts_with('#') {
>          comments.push_str(&line[1..]);
>          comments.push('\n');
>      }
> 
>      for (lineno, line) in lines {
>      <...>
> 

Done. Thanks :-)

>> -- snip --
>>
>> +    if object.contains_key("comment") {
>> +        let comment = match object.get("comment") {
> 
> `contains_key` + `get` is somewhat wasteful and should be combined.
> 
>      if let Some(comment) = object.get("comment") {
> 
> For the type check you can then use `.as_str().ok_or_else(...)`
> 
> Or alternatively use a single match for both checks:
> 
>      match object.get("comment") {
>          Some(Value::String(comment)) => {
>              <the loop>
>          }
>          Some(_) => bail!(...),
>          None => (),
>      }
> 

Done. Combined it to:

      if let Some(Value::String(comment)) = object.get("comment") {
          /* loop here */
      }

Hope that's alright, personally I find that more legible than the match 
or `as_str().ok_or_else()`, although it is quite compact.

>> +            Some(Value::String(v)) => v,
>> +            _ => bail!("only strings can be comments"),
>> +        };
>> +
>> +        for lines in comment.lines() {
>> +            writeln!(output, "#{}", lines)?;
>> +        }
>> +    }
>> +
>>       for (key, value) in object.iter() {
> 
> Given that we type-check the comment above _and_ the data matching the
> schema is a precondition for calling this function, I'd just put an
> 
>      if key == "comment" { continue }
> 
> here rather than the conditional check limited to the `Value::String` case below.
> 

Coming back to rustfmt, it seems to want to format this as a multi-line 
if. So either, we ignore that and make it a single line if anyway, let 
rustfmt have its way, or we could do:

      match value {
              _ if key == "comment" => continue,
              Value::Null => continue,           // delete this entry
              /*..*/
      }

I'll switch it to whatever option is deemed nicer.

>> -- snip --
>> >> +
>> +#[test]
>> +fn test_with_comment() {
>> +    use proxmox_schema::ApiType;
>> +
>> +    // let's just reuse some schema we actually have available:
>> +    use crate::config::node::NodeConfig;
>> +
>> +    const NODE_INPUT: &str = "\
>> +        #this should\n\
>> +        #be included\n\
>> +        acme: account=pebble\n\
>> +        # this should not\n\
> 
> ^ I find it curous that your 'comment' section doesn't have leading
> spaces, while the "real comment" does ;-)
> 
> Should we think about trimming off up to 1 space when parsing the lines
> and prefix them with `"# "` when printing them out?
> Though I suppose it doesn't matter much as since we drop "real" comments
> on a RMW-cycle these files don't really need to bother much with being
> all that eye-pleasing I guess...

Note that in cases where indentation matters for Markdown rendering, 
such as lists, it is not trivial to distinguish between a space that is 
necessary for proper indentation and a space that a user wants for 
"cosmetic" reasons. It is currently valid Markdown to indent nested 
lists with only one space, i.e. the following note:

#- one
# - two

will render two as a nested list.

Further, AFAICT, that is how PVE handles these comments (just a 
preceding '#' no extra space etc.) [1,2]. I think there is some value in 
keeping this behavior consistent between products. My "real comment" has 
an extra space, because I find the lack of a space to be somewhat 
"claustrophobic" :-)

If someone where to add an extra space manually, that should still be 
parse-able. Extra spaces would be present, but not visible in the 
rendered HTML (due to how Markdown/HTML deal with extra spaces, with the 
exception of the cases mentioned above). From my testing that is 
consistent with PVE's behavior.

[1]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2369
[2]: 
https://git.proxmox.com/?p=qemu-server.git;a=blob;f=PVE/QemuServer.pm;h=a99f1a56f64a4789a9dc62184856bab2927d68c8;hb=HEAD#l2497