Merge lp:~evarlast/charm-tools/warn-description-newlines into lp:charm-tools/1.4

[Jay R. Wren]

  Warn on newlines in config description

  If there are newlines in a config options description,
  juju get <charmname> displays the newlines making for very ugly
  formatting.

  This patch causes charm-proof to warn on newlines in options description.

See also: https://bugs.launchpad.net/juju-core/+bug/1292116

[Charles Butler (lazypower)]

+1 LGTM

[Marco Ceppi (marcoceppi)]

NLGTM

[Tim Van Steenburgh (tvansteenburgh)]

I appreciate this patch very much, but I don't think this is the correct way to solve this problem.

I think imposing arbitrary restrictions on which yaml contructs can be used will lead to a surprising user experience. This problem should be addressed by fixing the way the data is displayed by `juju get`, not by imposing restrictions on the data itself.

[Charles Butler (lazypower)]

> +1 LGTM

And I've since been wayyy corrected about why this is not good. DNLGTM

Unmerged revisions

338. By Jay R. Wren on 2014-08-14

Warn on newlines in config description

If there are newlines in a config options description,
juju get <charmname> displays the newlines making for very ugly
formatting.

This patch causes charm-proof to warn on newlines in options description.

=== modified file 'charmtools/charms.py'
--- charmtools/charms.py	2014-06-17 15:44:04 +0000
+++ charmtools/charms.py	2014-08-14 21:53:52 +0000
@@ -182,6 +182,11 @@
                     self.warn(
                         'config.yaml: description of option %s should be a '
                         'non-empty string' % option_name)
+                if (isinstance(option_value['description'], basestring) and
+                        '\n' in option_value['description']):
+                    self.warn(
+                        'config.yaml: description of option {} contains '
+                        'newlines'.format(option_name))
             option_type = option_value.get('type', 'string')
             if option_type not in KNOWN_OPTION_TYPES:
                 self.warn('config.yaml: option %s has an invalid type (%s)'

=== modified file 'tests/test_charm_proof.py'
--- tests/test_charm_proof.py	2014-06-17 15:44:04 +0000
+++ tests/test_charm_proof.py	2014-08-14 21:53:52 +0000
@@ -341,6 +341,53 @@
             "for the tag 'tag:yaml.org,2002:python/name:__builtin__.int'")
         self.assertTrue(self.linter.lint[0].startswith(expected))
 
+    def test_multiline_description_newline_block_literal(self):
+        self.write_config("""
+            options:
+              string_opt:
+                description: |
+                  A long description over many lines.
+                  Please don't use pipe.
+                default: some text
+                type: string
+            """)
+        self.linter.check_config_file(self.charm_dir)
+        self.assertEqual(
+            ['W: config.yaml: description of option string_opt contains '
+             'newlines'],
+            self.linter.lint)
+
+    def test_multiline_description_newline_folded_block_literal(self):
+        self.write_config("""
+            options:
+              string_opt:
+                description: >
+                  A long description over many lines.
+                  Please don't use gt.
+                default: some text
+                type: string
+            """)
+        self.linter.check_config_file(self.charm_dir)
+        self.assertEqual(
+            ['W: config.yaml: description of option string_opt contains '
+             'newlines'],
+            self.linter.lint)
+
+    def test_multiline_description_does_not_have_newlines(self):
+        self.write_config("""
+            options:
+              string_opt:
+                description:
+                  A long description over many lines.
+                  Please don't use pipe.
+                default: some text
+                type: string
+            """)
+        self.linter.check_config_file(self.charm_dir)
+        self.assertEqual(
+            [],
+            self.linter.lint)
+
 
 class MaintainerValidationTest(TestCase):
     def test_two_maintainer_fields(self):