Use ruamel.yaml for YAMLEditable #302

skowalak · 2022-02-27T20:29:31Z

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

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

skowalak · 2022-02-27T20:35:12Z

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):

Pretty printing still relies on helpers.convert_to_yaml(). I don't know if that level of indentation/alignment is possible with ruamel
Values will be inserted in the standard template. I don't know if this breaks some workflows involving custom new user templates (Make YAML template configurable #146).
Helper tests still use helpers.convert_to_yaml(). I will add some tests for the new methods when I have some more time.

lucc

I did a quick review of the helper code, just to give you an idea. I will of course do another one at least when you remove the draft state.

khard/helpers/__init__.py

lucc · 2022-02-27T21:41:43Z

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 · 2022-02-28T21:23:11Z

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 · 2022-03-08T22:11:57Z

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.

I ran mypy
I ran pylint
I wrote a test for Normalise (quote) strings when editing through the Yaml interface? #276
I successfully ran the tests

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

lucc

You can skip the broken test like this:

diff --git a/test/test_command_line_interface.py b/test/test_command_line_interface.py
index cac683c..97b5ecf 100644
--- a/test/test_command_line_interface.py
+++ b/test/test_command_line_interface.py
@@ -480,6 +480,7 @@ class Merge(unittest.TestCase):
         self.assertEqual('contact2.vcf', second)


+@unittest.skip("reason for skipping")
 class AddEmail(unittest.TestCase):

     @TmpConfig(["contact1.vcf", "contact2.vcf"])

If I do that (temporarily!) the other tests all run fine.

I noticed that the birthday field and phone numbers as well as ZIP codes are now wrapped in quotes. Do you know if there is a way to only quote strings that are problematic (like the @ at the beginning)?

lucc · 2022-03-08T22:56:14Z

khard/helpers/__init__.py

@@ -61,6 +62,121 @@ def get_random_uid() -> str:
    return ''.join([random.choice(string.ascii_lowercase + string.digits)
                    for _ in range(36)])

+def yaml_clean(value: Optional[Union[str, List, Dict[str, Any], Sequence]]
+              ) -> Optional[Union[Sequence, List, str, Dict[str, Any],


Why did you add Sequence to the type signature? And if it is really needed you can leave out List I think.

I got a mypy error without the Sequence, meaning one of the properties of YAMLEditable that are used in to_yaml is of a Sequence type. I removed List though, thank you.

lucc · 2022-03-08T22:58:12Z

khard/helpers/__init__.py

+
+def yaml_dicts(
+        data: Optional[Dict[str, Any]],
+        defaults: Optional[Union[Dict[str, Any], List[str]]] = None


For Optional[Union[...]] it is nicer to write Union[..., None].

lucc · 2022-03-08T23:06:45Z

khard/carddav_object.py

+        }
+        template = helpers.get_new_contact_template()
+        yaml = YAML()
+        yaml.top_level_colon_align = True


I do not like the way this alignment looks so I would leave it out.

lucc · 2022-03-08T23:10:00Z

khard/carddav_object.py

-                    "Note", self.notes, 0, 5, True)
-            elif line.lower().startswith("webpage"):
-                strings += helpers.convert_to_yaml(
-                    "Webpage", self.webpages, 0, 8, True)


I like this code reduction :)

skowalak · 2022-03-09T11:10:45Z

@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 · 2022-03-09T19:29:28Z

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.

Use ruamel.yaml for YAMLEditable

9ab66ac

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

skowalak mentioned this pull request Feb 27, 2022

Normalise (quote) strings when editing through the Yaml interface? #276

Closed

lucc reviewed Feb 27, 2022

View reviewed changes

skowalak added 2 commits February 28, 2022 21:39

implement proposed changes by @lucc #302

b348938

fix mypy and pylint errors

a128473

skowalak marked this pull request as ready for review February 28, 2022 21:23

add unit test for #276

70b3a39

lucc requested changes Mar 8, 2022

View reviewed changes

fix type annotations, remove colon alignment in YAML output

ee7de87

lucc approved these changes Mar 9, 2022

View reviewed changes

lucc merged commit ef8868c into lucc:develop Mar 9, 2022

skowalak deleted the feature/276-yaml-generator branch March 9, 2022 21:48

lucc mentioned this pull request Sep 6, 2023

Fix yaml conversion of post addresses #324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ruamel.yaml for YAMLEditable #302

Use ruamel.yaml for YAMLEditable #302

skowalak commented Feb 27, 2022

skowalak commented Feb 27, 2022 •

edited

Loading

lucc left a comment

lucc commented Feb 27, 2022

skowalak commented Feb 28, 2022 •

edited

Loading

skowalak commented Mar 8, 2022 •

edited

Loading

lucc left a comment

lucc Mar 8, 2022

skowalak Mar 9, 2022

lucc Mar 8, 2022

lucc Mar 8, 2022

lucc Mar 8, 2022

skowalak commented Mar 9, 2022

lucc commented Mar 9, 2022

Use ruamel.yaml for YAMLEditable #302

Use ruamel.yaml for YAMLEditable #302

Conversation

skowalak commented Feb 27, 2022

skowalak commented Feb 27, 2022 • edited Loading

lucc left a comment

Choose a reason for hiding this comment

lucc commented Feb 27, 2022

skowalak commented Feb 28, 2022 • edited Loading

skowalak commented Mar 8, 2022 • edited Loading

lucc left a comment

Choose a reason for hiding this comment

lucc Mar 8, 2022

Choose a reason for hiding this comment

skowalak Mar 9, 2022

Choose a reason for hiding this comment

lucc Mar 8, 2022

Choose a reason for hiding this comment

lucc Mar 8, 2022

Choose a reason for hiding this comment

lucc Mar 8, 2022

Choose a reason for hiding this comment

skowalak commented Mar 9, 2022

lucc commented Mar 9, 2022

skowalak commented Feb 27, 2022 •

edited

Loading

skowalak commented Feb 28, 2022 •

edited

Loading

skowalak commented Mar 8, 2022 •

edited

Loading