Hi Daniel,
This is almost good to go in, some minor things below:
On Mon, Nov 21, 2011 at 03:44:41PM +0100, Daniel Dehennin wrote:
[..snip..]
> -def fixup_trailer(repo, git_author, dch_options):
> +def fixup_section(repo, git_author, options=[], dch_options=[]):
It seems you always pass in all parameters so no need for default values
here.
> """
> - Fixup the changelog trailer's comitter and email address. > + Fixup
> the changelog header and trailer's comitter and email address
>
> It might otherwise point to the last git committer instead of the person
> creating the changelog
> + This apply --distribution and --urgency options passed to git-dch
> """
> author, email = get_author_email(repo, git_author)
> - ChangeLog.spawn_dch(msg='', author=author, email=email,
> dch_options=dch_options)
> + used_options = ['distribution']
> + header_opts = []
> +
> + # This must not be done for snapshots or snapshots changelog entries
> + # will not be concatenated
> + if not options.snapshot:
> + for opt in used_options:
> + if hasattr(options, opt) and getattr(options, opt):
Why can't you just do a getattr(options. opt)? The option should always
be there (but the value might be None). It would also be nice to use a
variable like:
val = getattr(options, opt)
if val:
...
header_opts.append("--%s=%s" % (opt, val))
instead of invoking getattr several times which is hard to read.
> + gbp.log.debug( "Set header option '%s' to '%s'" % (opt,
> getattr(options,opt)) )
> + header_opts.append("--%s=%s" % (opt, getattr(options,opt)))
> + else:
> + gbp.log.debug("Snapshot enabled: do not fixup options in header")
> +
> + ChangeLog.spawn_dch(msg='', author=author, email=email,
> dch_options=dch_options+header_opts)
>
>
> def snapshot_version(version):
> @@ -207,6 +221,9 @@ def process_options(options, parser):
> else:
> dch_options.append("--nomultimaint")
>
> + if options.force_distribution:
> + dch_options.append("--force-distribution")
> +
> get_customizations(options.customization_file)
> return dch_options
>
> @@ -279,6 +296,9 @@ def main(argv):
> help="mark as release")
> version_group.add_option("-S", "--snapshot", action="store_true",
> dest="snapshot", default=False,
> help="mark as snapshot build")
> + version_group.add_option("-D", "--distribution", dest="distribution",
> help="Set distribution")
> + version_group.add_option("--force-distribution", action="store_true",
> dest="force_distribution", default=False,
> + help="Force the provided distribution to be used, even
> if it doesn't match the list of known distributions")
> version_group.add_option("-N", "--new-version", dest="new_version",
> help="use this as base for the new version number")
> version_group.add_option("--bpo", dest="bpo", action="store_true",
> default=False,
> @@ -430,7 +450,7 @@ def main(argv):
> version=version_change,
> dch_options=dch_options)
>
> - fixup_trailer(repo, git_author=options.git_author,
> + fixup_section(repo, git_author=options.git_author, options=options,
> dch_options=dch_options)
>
> if options.release:
> diff --git a/tests/11_test_dch_main.py b/tests/11_test_dch_main.py
> index f45857e..9b40411 100644
> --- a/tests/11_test_dch_main.py
> +++ b/tests/11_test_dch_main.py
> @@ -194,6 +194,109 @@ class TestScriptDch(DebianGitTestRepo):
> self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> self.assertRaises(SystemExit, dch.main, options)
>
> + def test_dch_main_new_upstream_version_with_distribution(self):
> + """Test dch.py like git-dch script does: new upstream version - set
> distribution"""
> + options = self.options[:]
> + options.append("--distribution=testing")
> + self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
This is almost the same in all tests. Could you move this into a common
functtion to ease readability and reduce the amout of copy/paste code?
Seems this only differs in the options passed, so this could be made the
function argument. and the function could return the result of
readlines()
> + self.assertEqual("test-package (1.0-1) testing; urgency=low\n",
> lines[0])
> + self.assertIn(""" * added debian/control\n""", lines)
> +
> + def test_dch_main_new_upstream_version_with_release_distribution(self):
> + """Test dch.py like git-dch script does: new upstream version -
> release - set distribution"""
> + options = self.options[:]
> + options.append("--release")
> + options.append("--distribution=testing")
> + self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + self.assertEqual("test-package (1.0-1) testing; urgency=low\n",
> lines[0])
> + self.assertIn(""" * added debian/control\n""", lines)
> +
> + def test_dch_main_new_upstream_version_with_snapshot_distribution(self):
> + """Test dch.py like git-dch script does: new upstream version -
> snashot mode - do not set distribution"""
> + options = self.options[:]
> + options.append("--snapshot")
> + options.append("--distribution=testing")
> + self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
See above.
> + header = re.search(snap_header_1, lines[0])
> + self.assertIsNotNone(header)
> + self.assertEqual(header.lastindex, 1)
> + self.assertIsNotNone(re.search(snap_mark + header.group(1),
> lines[2]))
> + self.assertIn(""" * added debian/control\n""", lines)
> +
> + def
> test_dch_main_new_upstream_version_with_2_snapshots_auto_distribution(self):
> + """Test dch.py like git-dch script does: new upstream version - two
> snapshots - do not set distribution"""
> + options = self.options[:]
> + options.append("--snapshot")
> + options.append("--distribution=testing")
> + self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
See above.
> + header1 = re.search(snap_header_1, lines[0])
> + self.assertIsNotNone(header1)
> + self.assertEqual(header1.lastindex, 1)
> + self.assertIsNotNone(re.search(snap_mark + header1.group(1),
> lines[2]))
> + self.assertIn(""" * added debian/control\n""", lines)
> + # New snapshot, use auto to guess last one
> + options.append("--auto")
> + self.add_file("debian/compat", "9")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header2 = re.search(snap_header_1_2, lines[0])
> + self.assertIsNotNone(header2)
> + self.assertEqual(header2.lastindex, 1)
> + self.assertIsNotNone(re.search(snap_mark + header2.group(1),
> lines[2]))
> + # First snapshot entry must be concatenated with the last one
> + self.assertNotIn(header1.group(0) + "\n", lines)
> + self.assertIn(""" * added debian/control\n""", lines)
> + self.assertIn(""" * added debian/compat\n""", lines)
> + # But its changelog must not be included in the new one since
> + # we do not commit
> + self.assertNotIn(""" * TEST-COMMITTED-SNAPSHOT\n""", lines)
> +
> + def
> test_dch_main_new_upstream_version_with_2_snapshots_commit_auto_distribution(self):
> + """Test dch.py like git-dch script does: new upstream version - two
> committed snapshots - do not set distribution"""
> + options = self.options[:]
> + options.append("--commit")
> + options.append("--commit-msg=TEST-COMMITTED-SNAPSHOT")
> + options.append("--snapshot")
> + options.append("--distribution=testing")
> + self.repo.create_tag("debian/0.9-1", msg="Pre stable release version
> 0.9-1", commit="HEAD~1")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
See above. The same hold for the tests in the urgency management follow
up patch.
Cheers and thanks for your patience!
-- Guido
> + header1 = re.search(snap_header_1, lines[0])
> + self.assertIsNotNone(header1)
> + self.assertEqual(header1.lastindex, 1)
> + self.assertIsNotNone(re.search(snap_mark + header1.group(1),
> lines[2]))
> + self.assertIn(""" * added debian/control\n""", lines)
> + # New snapshot, use auto to guess last one
> + options.append("--auto")
> + self.add_file("debian/compat", "9")
> + ret = dch.main(options)
> + self.assertEqual(ret, 0)
> + lines = file("debian/changelog").readlines()
> + header2 = re.search(snap_header_1_2, lines[0])
> + self.assertIsNotNone(header2)
> + self.assertEqual(header2.lastindex, 1)
> + self.assertIsNotNone(re.search(snap_mark + header2.group(1),
> lines[2]))
> + self.assertIn(""" * added debian/control\n""", lines)
> + self.assertIn(""" * added debian/compat\n""", lines)
> + # First snapshot entry must have disapear
> + self.assertNotIn(header1.group(0) + "\n", lines)
> + # But its changelog must be included in the new one
> + self.assertIn(""" * TEST-COMMITTED-SNAPSHOT\n""", lines)
> +
> def test_dch_main_increment_debian_version(self):
> """Test dch.py like git-dch script does: increment debian version"""
> options = self.options[:]
> --
> 1.7.10.4
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
