lucc / khard

Console vcard client
https://khard.readthedocs.io/en/latest/
GNU General Public License v3.0
595 stars 66 forks source link

Use ruamel.yaml for YAMLEditable #302

Closed skowalak closed 2 years ago

skowalak commented 2 years ago

replace the YAMLEditable.to_yaml() method with ruamel serialization. migrate some yaml formatting to the helpers module.

skowalak commented 2 years ago

This PR references (#276) The entire YAMLEditable.to_yaml() function is rewritten to use ruamel.yaml. This fixes the problem of having illegal yaml values such as unquoted @ chars at the beginning of the value.

What I have not done (yet):

lucc commented 2 years ago

I think you need not bother with #146 because that issue is still in the discussion and planing phase. You can also leave the pretty printing alone as we just reuse the yaml conversion functions. We do not officially claim that the output of the pretty printer is valid yaml. We can thing about changing the pretty printer in another PR.

Please try to run the tests and mypy (and maybe even pylint) locally because currently travis integration is broken. (I need help from @scheibler to fix this.)

If you could also write a test for #276 (the string with an @) that would be great!

skowalak commented 2 years ago

I have a little difficulty running the tests. When I run python setup.py test hangs at the line

test_contact_is_found_if_name_matches (test.test_command_line_interface.AddEmail) ... 

I have to interrupt the testing via CTRL-C because the process' memory usage grows beyond what I consider normal (10GiB). Is that correct? Could my changes have caused this behaviour, if incorrect?

Edit: Here is a more detailed log from unittest discover

  File "/home/skowalak/khard/test/test_command_line_interface.py", line 498, in test_contact_is_found_if_name_matches
    run_main("add-email", "--input-file", tmp.name)
  File "/home/skowalak/khard/test/test_command_line_interface.py", line 33, in run_main
    khard.main(args)
  File "/home/skowalak/khard/khard/khard.py", line 1120, in main
    add_email_subcommand(input_from_stdin_or_file,
  File "/home/skowalak/khard/khard/khard.py", line 682, in add_email_subcommand
    add_email_to_contact(name, address, abooks, skip_already_added)
  File "/home/skowalak/khard/khard/khard.py", line 478, in add_email_to_contact
    .format(selected_vcard))
KeyboardInterrupt
skowalak commented 2 years ago

Hi @lucc , after some troubleshooting the abovementioned issue, I tried running the tests on the develop branch and had the exact same problem (tests.test_command_line_interface.AddEmail hangs in an infinite loop consuming more and more memory). I even tried it on a different computer running a different OS.

So apparently my changes did not break the tests.

Is there anything more I need to do before this PR can be merged?

skowalak commented 2 years ago

@lucc I implemented the changes you requested. About the quoting, I am not entirely sure. I believe ZIP codes at least are quoted because they are strings consisting of numbers only and ruamel quotes them so they are not interpreted as tag:yaml.org,2002:int, which would for example remove leading zeroes. That does not apply to birthdays (as they include dashes), still they are quoted, when according to the yaml spec they do not have be quoted. I had a look over at ruamels source code but could not find out why.

So, if that is a big problem we could ofc ask Anthon van der Neut from ruamel if he knows if it is possible not to quote this.

lucc commented 2 years ago

thank you @skowalak !

We can leave the quoting for later. That is just aesthetics in my opinion. The important thing is that yaml editing should be more robust now that #276 should be fixed.