PR#219: Add a convert subcommand - fedora-infra/rpmautospec

		`@@ -3,9 +3,10 @@`
		`import sys`
		`import typing`

		`- from .subcommands import changelog, process_distgit, release`
		`+ from .subcommands import changelog, convert, process_distgit, release`


		`+ all_subcmds = (changelog, convert, process_distgit, release)`
		`subcmd_modules_by_name = {}`


		`@@ -75,12 +76,10 @@`
		`# parsers for sub-commands`

		# ArgumentParser.add_subparsers() only accepts the `required` argument from Python 3.7 on.
		`- subparsers = parser.add_subparsers(`
		`- dest="subcommand", metavar="{generate-changelog,calculate-release}"`
		`- )`
		`+ subparsers = parser.add_subparsers(dest="subcommand")`
		`subparsers.required = True`

		`- for subcmd_module in (changelog, release, process_distgit):`
		`+ for subcmd_module in all_subcmds:`
		`subcmd_name = subcmd_module.register_subcommand(subparsers)`

		`if subcmd_name in subcmd_modules_by_name:`

rpmautospec/subcommands/convert.py

file added

+220

		`@@ -0,0 +1,220 @@`
		`+ import logging`
		`+ from pathlib import Path`
		`+ import re`
		`+`
		`+ import pygit2`
		`+`
		`+ from ..misc import autochangelog_re, autorelease_re, changelog_re`
		`+`
		`+`
		`+ log = logging.getLogger(__name__)`
		`+`
		`+`
		`+ class PkgConverter:`
		`+ def __init__(self, spec_or_path: Path):`
		`+ spec_or_path = spec_or_path.absolute()`
		`+`
		`+ if not spec_or_path.exists():`
		`+ raise RuntimeError(f"Spec file or path {str(spec_or_path)!r} doesn't exist")`
		`+ elif spec_or_path.is_dir():`
		`+ self.path = spec_or_path`
		`+ name = spec_or_path.name`
		`+ self.specfile = spec_or_path / f"{name}.spec"`
		`+ elif spec_or_path.is_file():`
		`+ if spec_or_path.suffix != ".spec":`
		`+ raise ValueError(`
		`+ f"Spec file {str(spec_or_path)!r} must have '.spec' as an extension"`
		`+ )`
		`+ self.path = spec_or_path.parent`
		`+ self.specfile = spec_or_path`
		`+ else:`
		`+ raise RuntimeError(f"Spec file or path {str(spec_or_path)!r} is not a regular file")`
		`+`
		`+ if not self.specfile.exists():`
		`+ raise RuntimeError(`
		`+ f"Spec file {str(self.specfile)!r} doesn't exist in {str(self.path)!r}"`
		`+ )`
		`+`
		`+ log.debug("Working on spec file %s", self.specfile)`
		`+`
		`+ try:`
		`+ if hasattr(pygit2, "GIT_REPOSITORY_OPEN_NO_SEARCH"):`
		`+ kwargs = {"flags": pygit2.GIT_REPOSITORY_OPEN_NO_SEARCH}`
		`+ else:`
		`+ # pygit2 < 1.4.0`
		`+ kwargs = {}`
		`+ self.repo = pygit2.Repository(self.path, **kwargs)`
		`+ log.debug("Found repository at %s", self.path)`
		`+ except pygit2.GitError:`
		`+ self.repo = None`
		`+ log.debug("Found no repository at %s", self.path)`
		`+`
		`+ if self.repo is not None:`
		`+ try:`
		`+ spec_status = self.repo.status_file(self.specfile.relative_to(self.path))`
		`+ except KeyError:`
		`+ raise RuntimeError(`
		`+ f"Spec file {str(self.specfile)!r} is in a repository, but isn't tracked"`
		`+ )`
		`+ # Allow converting unmodified, and saved-to-index files.`
		`+ if spec_status not in (pygit2.GIT_STATUS_CURRENT, pygit2.GIT_STATUS_INDEX_MODIFIED):`
		`+ raise RuntimeError(f"Spec file {str(self.specfile)!r} is modified")`
		`+ try:`
		`+ self.repo.status_file("changelog")`
		`+ except KeyError:`
		`+ pass # Doesn't exist; good.`
		`+ else:`
		`+ raise RuntimeError("Changelog file 'changelog' is already in the repository")`
		`+ for filepath, flags in self.repo.status().items():`
		`+ if flags not in (`
		`+ pygit2.GIT_STATUS_CURRENT,`
		`+ pygit2.GIT_STATUS_IGNORED,`
		`+ pygit2.GIT_STATUS_WT_NEW,`
		`+ ):`
		`+ raise RuntimeError(f"Repository '{str(self.path)!r}' is dirty")`
		`+`
		`+ self.changelog_lines = None`
		`+ self.spec_lines = None`
		`+`
		`+ def load(self):`
		`+ with self.specfile.open(encoding="utf-8") as f:`
		`+ self.spec_lines = f.readlines()`
		`+`
		`+ def save(self):`
		`+ with self.specfile.open("w", encoding="utf-8") as f:`
		`+ f.writelines(self.spec_lines)`
		`+ if self.changelog_lines is not None:`
		`+ with (self.path / "changelog").open("w", encoding="utf-8") as f:`
		`+ f.writelines(self.changelog_lines)`
		`+`
		`+ def convert_to_autorelease(self):`
		`+ release_re = re.compile(r"^(?P<tag>Release\s:\s)", re.IGNORECASE)`
		`+ release_lines = ((i, release_re.match(line)) for i, line in enumerate(self.spec_lines))`
		`+ release_lines = [(i, match) for i, match in release_lines if match]`
		`+ line_numbers = ", ".join(f"{i+1}" for i, _ in release_lines)`
		`+ log.debug("Found Release tag on line(s) %s", line_numbers)`
		`+`
		`+ if not release_lines:`
		`+ raise RuntimeError(f"Unable to locate Release tag in spec file {str(self.specfile)!r}")`
		`+ elif len(release_lines) > 1:`
		`+ raise RuntimeError(`
		`+ f"Found multiple Release tags on lines {line_numbers} "`
		`+ f"in spec file {str(self.specfile)!r}"`
		`+ )`
		`+`
		`+ # Process the line so the inserted macro is aligned to the previous location of the tag.`
		`+ lineno, match = release_lines[0]`
		`+ if autorelease_re.match(self.spec_lines[lineno]):`
		`+ log.warning(f"{str(self.specfile)!r} is already using %autorelease")`
		`+ return`
		`+ self.spec_lines[lineno] = f"{match.group('tag')}%autorelease\n"`
		`+`
		`+ def convert_to_autochangelog(self):`
		`+ changelog_lines = [i for i, line in enumerate(self.spec_lines) if changelog_re.match(line)]`
		`+ line_numbers = ", ".join(f"{i+1}" for i in changelog_lines)`
		`+ log.debug("Found %%changelog on line(s) %s", line_numbers)`
		`+`
		`+ if not changelog_lines:`
		`+ raise RuntimeError(`
		`+ f"Unable to locate %changelog line in spec file {str(self.specfile)!r}"`
		`+ )`
		`+ elif len(changelog_lines) > 1:`
		`+ raise RuntimeError(`
		`+ f"Found multiple %changelog on lines {line_numbers} "`
		`+ f"in spec file {str(self.specfile)!r}"`
		`+ )`
		`+`
		`+ lineno = changelog_lines[0] + 1`
		`+ if autochangelog_re.match(self.spec_lines[lineno]):`
		`+ log.warning(f"{str(self.specfile)!r} is already using %autochangelog")`
		`+ return`
		`+ self.changelog_lines = self.spec_lines[lineno:]`
		`+ self.spec_lines[lineno:] = ["%autochangelog\n"]`
		`+ log.debug("Split %d lines to 'changelog' file", len(self.changelog_lines))`
		`+`
		`+ def commit(self, message: str):`
		`+ if self.repo is None:`
		`+ log.debug("Unable to open repository at '%s'", self.path)`
		`+ return`
		`+`
		`+ index = self.repo.index`
		`+ index.add(self.specfile.relative_to(self.path))`
		`+ if self.changelog_lines is not None:`
		`+ index.add("changelog")`
		`+ index.write()`
		`+ tree = index.write_tree()`
		`+`
		`+ parent, ref = self.repo.resolve_refish(refish=self.repo.head.name)`
		`+ signature = self.repo.default_signature`
		`+ log.debug(`
		`+ "Committing tree %s with author '%s <%s>' on branch '%s'",`
		`+ tree,`
		`+ signature.name,`
		`+ signature.email,`
		`+ ref.shorthand,`
		`+ )`
		`+ oid = self.repo.create_commit(ref.name, signature, signature, message, tree, [parent.oid])`
		`+ log.debug("Committed %s to repository", oid)`
		`+`
		`+`
		`+ def register_subcommand(subparsers):`
		`+ subcmd_name = "convert"`
		`+`
		`+ convert_parser = subparsers.add_parser(`
		`+ subcmd_name,`
		`+ help="Convert a package repository to use rpmautospec",`
		`+ )`
		`+`
		`+ convert_parser.add_argument(`
		`+ "spec_or_path",`
		`+ default=".",`
		`+ nargs="?",`
		`+ help="Path to package worktree or spec file",`
		`+ )`
		`+`
		`+ convert_parser.add_argument(`
		`+ "--message",`
		`+ "-m",`
		`+ default="Convert to rpmautospec",`
		`+ help="Message to use when committing changes",`
		`+ )`
		`+`
		`+ convert_parser.add_argument(`
		`+ "--no-commit",`
		`+ "-n",`
		`+ action="store_true",`
		`+ help="Don't commit after making changes",`
		`+ )`
		`+`
		`+ convert_parser.add_argument(`
		`+ "--no-changelog",`
		`+ action="store_true",`
		`+ help="Don't convert the %%changelog",`
		`+ )`
		`+`
		`+ convert_parser.add_argument(`
		`+ "--no-release",`
		`+ action="store_true",`
		`+ help="Don't convert the Release field",`
		`+ )`
		`+`
		`+ return subcmd_name`
		`+`
		`+`
		`+ def main(args):`
		`+ """Main method."""`
		`+ if not args.no_commit:`
		`+ if not args.message:`
		`+ raise RuntimeError("Commit message cannot be empty")`
		`+ if args.no_changelog and args.no_release:`
		`+ raise RuntimeError("All changes are disabled")`
		`+`
		`+ pkg = PkgConverter(Path(args.spec_or_path))`
		`+ pkg.load()`
		`+ if not args.no_changelog:`
		`+ pkg.convert_to_autochangelog()`
		`+ if not args.no_release:`
		`+ pkg.convert_to_autorelease()`
		`+ pkg.save()`
		`+ if not args.no_commit:`
		`+ pkg.commit(args.message)`

tests/conftest.py

file added

+95

		`@@ -0,0 +1,95 @@`
		`+ import pygit2`
		`+ import pytest`
		`+`
		`+`
		`+ SPEC_FILE_TEXT = """Summary: Boo`
		`+ Name: boo`
		`+ Version: 1.0`
		`+ {release}`
		`+ License: CC0`
		`+`
		`+ %description`
		`+ Boo`
		`+`
		`+ {changelog}`
		`+ """`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def changelog(request):`
		`+ """`
		`+ This fixture exists to be substituted into the specfile fixture`
		`+ indirectly, or else provide a default of %autochangelog.`
		`+ """`
		`+ return getattr(request, "param", "%changelog\n%autochangelog")`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def release(request):`
		`+ """`
		`+ This fixture exists to be substituted into the specfile fixture`
		`+ indirectly, or else provide a default of %autorelease.`
		`+ """`
		`+ return getattr(request, "param", "Release: %autorelease")`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def repopath(tmp_path):`
		`+ repopath = tmp_path / "test"`
		`+ repopath.mkdir()`
		`+`
		`+ yield repopath`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def specfile(repopath, release, changelog):`
		`+ """`
		`+ Generate a spec file within repopath.`
		`+`
		`+ The Release tag will be replaced by the release fixture, if defined, or`
		`+ else will be filled by %autorelease. The changelog will be replaced by the`
		`+ changelog fixture, if defined, or else will be filled by %autochangelog.`
		`+ """`
		`+`
		`+ specfile = repopath / "test.spec"`
		`+ specfile.write_text(SPEC_FILE_TEXT.format(release=release, changelog=changelog))`
		`+`
		`+ yield specfile`
		`+`
		`+`
		`+ @pytest.fixture`
		`+ def repo(repopath, specfile):`
		`+ pygit2.init_repository(repopath, initial_head="rawhide")`
		`+ if hasattr(pygit2, "GIT_REPOSITORY_OPEN_NO_SEARCH"):`
		`+ repo = pygit2.Repository(repopath, pygit2.GIT_REPOSITORY_OPEN_NO_SEARCH)`
		`+ else:`
		`+ # pygit2 < 1.4.0`
		`+ repo = pygit2.Repository(repopath)`
		`+`
		`+ repo.config["user.name"] = "Jane Doe"`
		`+ repo.config["user.email"] = "jane.doe@example.com"`
		`+`
		`+ # create root commit in "rawhide" branch`
		`+ index = repo.index`
		`+ index.add(specfile.name)`
		`+ index.write()`
		`+`
		`+ tree = index.write_tree()`
		`+`
		`+ oid = repo.create_commit(`
		`+ None, repo.default_signature, repo.default_signature, "Initial commit", tree, []`
		`+ )`
		`+ repo.branches.local.create("rawhide", repo[oid])`
		`+`
		`+ # add another commit (empty)`
		`+ parent, ref = repo.resolve_refish(repo.head.name)`
		`+ repo.create_commit(`
		`+ ref.name,`
		`+ repo.default_signature,`
		`+ repo.default_signature,`
		`+ "Did nothing!",`
		`+ tree,`
		`+ [parent.oid],`
		`+ )`
		`+`
		`+ yield repo`

tests/rpmautospec/subcommands/test_convert.py

file added

+282

		`@@ -0,0 +1,282 @@`
		`+ """`
		`+ Test the rpmautospec.subcommands.converter module`
		`+ """`
		`+`
		`+ import logging`
		`+ from types import SimpleNamespace`
		`+ import unittest.mock`
		`+`
		`+ import pygit2`
		`+ import pytest`
		`+`
		`+ from rpmautospec.subcommands import convert`
		`+ from rpmautospec.misc import autorelease_re, autochangelog_re`
		`+`
		`+`
		`+ def test_init_invalid_path(tmp_path):`
		`+ with pytest.raises(RuntimeError, match="doesn't exist"):`
		`+ convert.PkgConverter(tmp_path / "nonexistent.spec")`
		`+`
		`+ dir_no_spec = tmp_path / "dir_no_spec"`
		`+ dir_no_spec.mkdir()`
		`+ with pytest.raises(RuntimeError, match="doesn't exist in "):`
		`+ convert.PkgConverter(dir_no_spec)`
		`+`
		`+ no_spec_extension = tmp_path / "noext"`
		`+ no_spec_extension.touch()`
		`+ with pytest.raises(ValueError, match="must have '.spec' as an extension"):`
		`+ convert.PkgConverter(no_spec_extension)`
		`+`
		`+`
		`+ def test_init_dirty_tree(specfile, repo):`
		`+ # The changelog file has already been added:`
		`+ changelog = specfile.parent / "changelog"`
		`+ changelog.touch()`
		`+ with pytest.raises(RuntimeError, match="'changelog' is already in the repository"):`
		`+ convert.PkgConverter(specfile)`
		`+ changelog.unlink()`
		`+`
		`+ # The spec file has been modified without committing it:`
		`+ specfile.write_text("Modified")`
		`+ with pytest.raises(RuntimeError, match="is modified"):`
		`+ convert.PkgConverter(specfile)`
		`+ repo.reset(repo.head.target, pygit2.GIT_RESET_HARD)`
		`+`
		`+ # Other files have been changed, which may corrupt our commit:`
		`+ dirty = specfile.parent / "dirty"`
		`+ dirty.write_text("")`
		`+ repo.index.add("dirty")`
		`+ repo.index.write()`
		`+ with pytest.raises(RuntimeError, match="is dirty"):`
		`+ convert.PkgConverter(specfile)`
		`+`
		`+`
		`+ @unittest.mock.patch("rpmautospec.subcommands.convert.PkgConverter")`
		`+ def test_main_invalid_args(specfile):`
		`+ args = SimpleNamespace(`
		`+ spec_or_path=specfile, message="", no_commit=False, no_changelog=False, no_release=False`
		`+ )`
		`+ with pytest.raises(RuntimeError, match="Commit message cannot be empty"):`
		`+ convert.main(args)`
		`+`
		`+ args = SimpleNamespace(`
		`+ spec_or_path=specfile,`
		`+ message="message",`
		`+ no_commit=False,`
		`+ no_changelog=True,`
		`+ no_release=True,`
		`+ )`
		`+ with pytest.raises(RuntimeError, match="All changes are disabled"):`
		`+ convert.main(args)`
		`+`
		`+`
		`+ @unittest.mock.patch("rpmautospec.subcommands.convert.PkgConverter")`
		`+ def test_main_valid_args(pkg_converter_mock, specfile):`
		`+ pkg_converter = unittest.mock.Mock(spec=convert.PkgConverter)()`
		`+ pkg_converter_mock.return_value = pkg_converter`
		`+`
		`+ # No Release change.`
		`+ pkg_converter.reset_mock()`
		`+ args = SimpleNamespace(`
		`+ spec_or_path=specfile,`
		`+ message="message",`
		`+ no_commit=False,`
		`+ no_changelog=False,`
		`+ no_release=True,`
		`+ )`
		`+ convert.main(args)`
		`+ pkg_converter.load.assert_called_once()`
		`+ pkg_converter.convert_to_autochangelog.assert_called_once()`
		`+ pkg_converter.convert_to_autorelease.assert_not_called()`
		`+ pkg_converter.save.assert_called()`
		`+ pkg_converter.commit.assert_called_once_with("message")`
		`+`
		`+ # No %changelog change.`
		`+ pkg_converter.reset_mock()`
		`+ args = SimpleNamespace(`
		`+ spec_or_path=specfile,`
		`+ message="message",`
		`+ no_commit=False,`
		`+ no_changelog=True,`
		`+ no_release=False,`
		`+ )`
		`+ convert.main(args)`
		`+ pkg_converter.load.assert_called_once()`
		`+ pkg_converter.convert_to_autochangelog.assert_not_called()`
		`+ pkg_converter.convert_to_autorelease.assert_called_once()`
		`+ pkg_converter.save.assert_called_once()`
		`+ pkg_converter.commit.assert_called_once_with("message")`
		`+`
		`+ # No git commit.`
		`+ pkg_converter.reset_mock()`
		`+ args = SimpleNamespace(`
		`+ spec_or_path=specfile,`
		`+ message="message",`
		`+ no_commit=True,`
		`+ no_changelog=False,`
		`+ no_release=False,`
		`+ )`
		`+ convert.main(args)`
		`+ pkg_converter.load.assert_called_once()`
		`+ pkg_converter.convert_to_autochangelog.assert_called_once()`
		`+ pkg_converter.convert_to_autorelease.assert_called_once()`
		`+ pkg_converter.save.assert_called_once()`
		`+ pkg_converter.commit.assert_not_called()`
		`+`
		`+`
		`+ @pytest.mark.parametrize(`
		`+ "release, expected",`
		`+ [`
		`+ ("Release: 1%{dist}", "\nRelease: %autorelease\n"),`
		`+ ("Release \t: \t1%{dist}", "\nRelease \t: \t%autorelease\n"),`
		`+ ("ReLeAsE: 1%{dist}", "\nReLeAsE: %autorelease\n"),`
		`+ ],`
		`+ ids=[`
		`+ "regular",`
		`+ "whitespace",`
		`+ "case",`
		`+ ],`
		`+ indirect=["release"],`
		`+ )`
		`+ def test_autorelease(specfile, expected):`
		`+ assert autorelease_re.search(specfile.read_text()) is None`
		`+`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+ converter.convert_to_autorelease()`
		`+ converter.save()`
		`+`
		`+ assert autorelease_re.search(specfile.read_text()).group() == expected`
		`+`
		`+`
		`+ def test_autorelease_already_converted(specfile, caplog):`
		`+ assert autorelease_re.search(specfile.read_text()) is not None`
		`+`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+`
		`+ caplog.clear()`
		`+ converter.convert_to_autorelease()`
		`+ assert len(caplog.records) == 1`
		`+ assert "is already using %autorelease" in caplog.records[0].message`
		`+`
		`+`
		`+ @pytest.mark.parametrize(`
		`+ "release, expected",`
		`+ [`
		`+ ("", "Unable to locate Release tag"),`
		`+ ("Release: 1%{dist}\nRelease: 2%{dist}", "Found multiple Release tags"),`
		`+ ],`
		`+ ids=[`
		`+ "missing",`
		`+ "multiple",`
		`+ ],`
		`+ indirect=["release"],`
		`+ )`
		`+ def test_autorelease_invalid(specfile, expected):`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+ with pytest.raises(RuntimeError, match=expected):`
		`+ converter.convert_to_autorelease()`
		`+`
		`+`
		`+ @pytest.mark.parametrize(`
		`+ "changelog",`
		`+ [`
		`+ "%changelog\n- log line",`
		`+ "%ChAnGeLoG\n- log line",`
		`+ ],`
		`+ ids=[`
		`+ "regular",`
		`+ "case",`
		`+ ],`
		`+ indirect=True,`
		`+ )`
		`+ def test_autochangelog(specfile):`
		`+ assert autochangelog_re.search(specfile.read_text()) is None`
		`+`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+ converter.convert_to_autochangelog()`
		`+ converter.save()`
		`+`
		`+ assert autochangelog_re.search(specfile.read_text()) is not None`
		`+ changelog = specfile.parent / "changelog"`
		`+ assert changelog.exists()`
		`+ assert changelog.read_text() == "- log line\n"`
		`+`
		`+`
		`+ def test_autochangelog_already_converted(specfile, caplog):`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+`
		`+ caplog.clear()`
		`+ converter.convert_to_autochangelog()`
		`+ assert len(caplog.records) == 1`
		`+ assert "is already using %autochangelog" in caplog.records[0].message`
		`+`
		`+`
		`+ @pytest.mark.parametrize(`
		`+ "changelog, expected",`
		`+ [`
		`+ ("", "Unable to locate %changelog line"),`
		`+ ("%changelog\n%changelog", "Found multiple %changelog on lines"),`
		`+ ],`
		`+ ids=[`
		`+ "missing",`
		`+ "multiple",`
		`+ ],`
		`+ indirect=["changelog"],`
		`+ )`
		`+ def test_autochangelog_invalid(specfile, expected):`
		`+ assert autochangelog_re.search(specfile.read_text()) is None`
		`+`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+ with pytest.raises(RuntimeError, match=expected):`
		`+ converter.convert_to_autochangelog()`
		`+`
		`+`
		`+ def test_commit_no_repo(specfile, caplog):`
		`+ converter = convert.PkgConverter(specfile)`
		`+ with caplog.at_level(logging.DEBUG):`
		`+ converter.commit("Convert to rpmautospec.")`
		`+ assert len(caplog.records) == 1`
		`+ assert "Unable to open repository" in caplog.records[0].message`
		`+`
		`+`
		`+ @pytest.mark.parametrize(`
		`+ "release",`
		`+ ["Release: 1%{dist}", "Release: %{autorelease}"],`
		`+ ids=["release1", "autorelease"],`
		`+ indirect=True,`
		`+ )`
		`+ @pytest.mark.parametrize(`
		`+ "changelog",`
		`+ ["%changelog\n- log line", "%changelog\n%autochangelog"],`
		`+ ids=["changelog", "autochangelog"],`
		`+ indirect=True,`
		`+ )`
		`+ def test_commit(specfile, release, changelog, repo):`
		`+ converter = convert.PkgConverter(specfile)`
		`+ converter.load()`
		`+ converter.convert_to_autochangelog()`
		`+ converter.convert_to_autorelease()`
		`+ converter.save()`
		`+ converter.commit("Convert to rpmautospec.")`
		`+`
		`+ for filepath, flags in repo.status().items():`
		`+ assert flags == pygit2.GIT_STATUS_CURRENT`
		`+`
		`+ changelog_should_change = autochangelog_re.search(changelog) is None`
		`+ release_should_change = autorelease_re.search(release) is None`
		`+ head = repo.revparse_single("HEAD")`
		`+ diff = repo.diff("HEAD^", head)`
		`+ fileschanged = {`
		`+ *(patch.delta.new_file.path for patch in diff),`
		`+ *(patch.delta.old_file.path for patch in diff),`
		`+ }`
		`+`
		`+ assert head.message == "Convert to rpmautospec."`
		`+ assert (specfile.name in fileschanged) == (changelog_should_change or release_should_change)`
		`+ assert ("changelog" in fileschanged) == changelog_should_change`

tests/rpmautospec/test_pkg_history.py

file modified

-71

		`@@ -10,77 +10,6 @@`
		`from rpmautospec.pkg_history import PkgHistoryProcessor`


		`- SPEC_FILE_TEXT = """Summary: Boo`
		`- Name: boo`
		`- Version: 1.0`
		`- Release: %autorelease`
		`- License: CC0`
		`-`
		`- %description`
		`- Boo`
		`-`
		`- %changelog`
		`- %autochangelog`
		`- """`
		`-`
		`-`
		`- @pytest.fixture`
		`- def repodir(tmp_path):`
		`- repodir = tmp_path / "test"`
		`- repodir.mkdir()`
		`-`
		`- yield repodir`
		`-`
		`-`
		`- @pytest.fixture`
		`- def specfile(repodir):`
		`- specfile = repodir / "test.spec"`
		`- specfile.write_text(SPEC_FILE_TEXT)`
		`-`
		`- yield specfile`
		`-`
		`-`
		`- @pytest.fixture`
		`- def repo(repodir, specfile):`
		`- # pygit2 < 1.2.0 can't cope with pathlib.Path objects`
		`- repopath = str(repodir)`
		`-`
		`- pygit2.init_repository(repopath, initial_head="rawhide")`
		`- if hasattr(pygit2, "GIT_REPOSITORY_OPEN_NO_SEARCH"):`
		`- repo = pygit2.Repository(repopath, pygit2.GIT_REPOSITORY_OPEN_NO_SEARCH)`
		`- else:`
		`- # pygit2 < 1.4.0`
		`- repo = pygit2.Repository(repopath)`
		`-`
		`- repo.config["user.name"] = "Jane Doe"`
		`- repo.config["user.email"] = "jane.doe@example.com"`
		`-`
		`- # create root commit in "rawhide" branch`
		`- index = repo.index`
		`- index.add(specfile.name)`
		`- index.write()`
		`-`
		`- tree = index.write_tree()`
		`-`
		`- oid = repo.create_commit(`
		`- None, repo.default_signature, repo.default_signature, "Initial commit", tree, []`
		`- )`
		`- repo.branches.local.create("rawhide", repo[oid])`
		`-`
		`- # add another commit (empty)`
		`- parent, ref = repo.resolve_refish(repo.head.name)`
		`- repo.create_commit(`
		`- ref.name,`
		`- repo.default_signature,`
		`- repo.default_signature,`
		`- "Did nothing!",`
		`- tree,`
		`- [parent.oid],`
		`- )`
		`-`
		`- yield repo`
		`-`
		`-`
		`@pytest.fixture`
		`def processor(repo):`
		`processor = PkgHistoryProcessor(repo.workdir)`

qulogic commented 2 years ago

This will translate an existing spec to spec+changelog and commit it. Options are available to skip some of these steps.

I have tested this out on a real package, but still need to write actual tests.

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

pre-commit : FAILURE in 2m 21s
pytest-rawhide : SUCCESS in 3m 19s
pytest-f34 : SUCCESS in 2m 13s
pytest-f33 : SUCCESS in 2m 20s
rpm-mock-build : SUCCESS in 5m 53s

3 new commits added

Add a convert command to change a spec to rpmautospec.
Sketch out arguments for a convert command.
Avoid defining all subcommands far away from definition.

2 years ago

zuul commented 2 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 42s
pytest-rawhide : SUCCESS in 3m 39s
pytest-f34 : SUCCESS in 2m 11s
pytest-f33 : SUCCESS in 2m 06s
rpm-mock-build : SUCCESS in 6m 48s

zbyszek commented on line 24 of rpmautospec/pkg_converter.py 2 years ago

if spec_or_path.suffix != ".spec":

Maybe relax that to '.spec' in spec_of_path? Sometimes people have .spec.in or similar in upstream projects. It'd still catch most cases where somebody passes a different file by mistake.

zbyszek commented on line 26 of rpmautospec/pkg_converter.py 2 years ago

"File specified as spec_or_path must have '.spec' as an extension."

Here and below: I think it's very useful to specify the actual argument given by the user in the error message.
Users sometimes will pass the file name in the wrong place, and then an error message that doesn't say what
string we're looking at can be confusing.

zbyszek commented on line 18 of rpmautospec/pkg_converter.py 2 years ago

raise RuntimeError(f"Spec file or path '{spec_or_path}' doesn't exist.")

Here and below: RuntimeError is rather unexpected in Python. (I don't know the codebase at all,
so if there's a reason, please ignore this comment.) I would expect FileNotFoundError to be used.

zbyszek commented on line 31 of rpmautospec/pkg_converter.py 2 years ago

raise RuntimeError("File specified as spec_or_path is not a regular file.")

OSError ?

zbyszek commented on line 76 of rpmautospec/pkg_converter.py 2 years ago

with self.specfile.open(encoding="utf-8") as f:

self.spec_lines = f.readlines()

with for reading isn't useful. Just self.spec_lines = self.specfile.open(...).readlines() is good enough.

zbyszek commented on line 85 of rpmautospec/pkg_converter.py 2 years ago

release_lines = [i for i, line in enumerate(self.spec_lines) if line.startswith("Release")]

Maybe change line.startswith(...) to re.match(r'^Release\s*:, line)`? This will avoid some false positives.

zbyszek commented on line 101 of rpmautospec/pkg_converter.py 2 years ago

Or maybe even make that:

matches = ((i, re.match(r'^Release\s*:\s*', line)) for i, line in enumerate(self.spec_lines))
release_lines = [(i, match) for i, match in matches if match]
...
lineno, match = release_lines[0]
...
release_tag = f'{match.group(0)}%autorelease\n'

zbyszek commented on line 91 of rpmautospec/pkg_converter.py 2 years ago

raise RuntimeError(f"Found multiple Release tags in spec file '{self.specfile}'.")

This should print the matching lines…

zbyszek commented 2 years ago

Looks great in general!

qulogic commented 2 years ago

The determination of spec_or_path was all copied from pkg_history.py, as was the use of RuntimeError everywhere. I assume there was some project reason to do so, or I would have used clearer exceptions.

zbyszek commented 2 years ago

The determination of spec_or_path was all copied from pkg_history.py, as was the use of RuntimeError everywhere. I assume there was some project reason to do so, or I would have used clearer exceptions.

Right, that's what I meant by "I don't know the codebase at all". I guess it's a Javaism, where you need to use RuntimeException to avoid enforced exception checking.

3 new commits added

Add a convert command to change a spec to rpmautospec.
Sketch out arguments for a convert command.
Avoid defining all subcommands far away from definition.

2 years ago

zuul commented 2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

pre-commit : FAILURE in 1m 54s
pytest-rawhide : SUCCESS in 3m 37s
pytest-f34 : SUCCESS in 2m 48s
pytest-f33 : SUCCESS in 2m 07s
rpm-mock-build : SUCCESS in 5m 35s

nphilipp commented 2 years ago

if spec_or_path.suffix != ".spec":

Maybe relax that to '.spec' in spec_of_path? Sometimes people have .spec.in or similar in upstream projects. It'd still catch most cases where somebody passes a different file by mistake.

I wouldn't – rpmautospec is tailored to dist-git style package repositories and expects there to be a spec file with that suffix.

"File specified as spec_or_path must have '.spec' as an extension."

Here and below: I think it's very useful to specify the actual argument given by the user in the error message.
Users sometimes will pass the file name in the wrong place, and then an error message that doesn't say what
string we're looking at can be confusing.

Yeah, there is bad precedent elsewhere in the code :wink:. Displaying the offending value is a good idea.

raise RuntimeError(f"Spec file or path '{spec_or_path}' doesn't exist.")

Here and below: RuntimeError is rather unexpected in Python. (I don't know the codebase at all,
so if there's a reason, please ignore this comment.) I would expect FileNotFoundError to be used.

Meh, RuntimeError is legit but a tad unspecific, what's worse: it isn't caught and processed, i.e. will just display a stack trace to the user. We shouldn't use OSError or FileNotFoundError, though, they're specifically for errors from system calls. An IMO better alternative here and elsewhere would be e.g. a custom UserArgumentError exception inherited from ValueError, which would then be caught and translated into a "nice" error message. Out of scope for this PR, though.

with self.specfile.open(encoding="utf-8") as f:

self.spec_lines = f.readlines()

with for reading isn't useful. Just self.spec_lines = self.specfile.open(...).readlines() is good enough.

Using the context manager is good practice: it closes the file object when the context is left instead of leaving this implicitly to the garbage collector. Leave it as is.

release_lines = [i for i, line in enumerate(self.spec_lines) if line.startswith("Release")]

Maybe change line.startswith(...) to re.match(r'^Release\s*:, line)`? This will avoid some false positives.

Let's catch more (maybe all) valid ways of setting the release, the parser is case-insensitive. Also, I'm not seeing that the code preserves whitespace before and (more importantly) after the colon, so we could reuse the regex for this, à la:

class PkgConverter:
    release_re = re.compile(r"^(?P<rel_prefix>Release\s*:\s*", re.IGNORECASE)
    ...
    def convert_to_autorelease(self):
        all_lines_matches = ((i, self.release_re.match(line)) for i, line in enumerate(self.spec_lines))
        release_lines_matches = [(i, match) for i, match in all_lines_matches if match]
        log.debug("Found release tag(s) on lines: %s", ", ".join(str(i + 1) for i, m in release_lines_matches))
        ...
        lineidx, match = release_lines_matches[0]
        self.spec_lines[lineidx] = f"{match.group('rel_prefix')}%autorelease\n"

I guess it's a Javaism, where you need to use RuntimeException to avoid enforced exception checking.

Nah, just a lack of time to do it properly when we implemented it. :wink:

Overall, it looks good to me, thanks for contributing! Some more requests:

Please move the code from pkg_converter.py into subcommands/convert.py – the reason for pkg_history.py is because it's shared code used in many places, i.e. three subcommands and the Koji plugin.
Run black over your changes (this should fix what Zuul flags in the pre-commit job).
I'd like to have tests for this, if you need help there, let me know.

Edited 2 years ago by nphilipp

qulogic commented 2 years ago

Let's catch more (maybe all) valid ways of setting the release, the parser is case-insensitive. Also, I'm not seeing that the code preserves whitespace before and (more importantly) after the colon,

It does preserve whitespace correctly; I did miss the case insensitivity though.

5 new commits added

Add tests for converter.
Move repo fixture to a common file.
Add a convert command to change a spec to rpmautospec.
Sketch out arguments for a convert command.
Avoid defining all subcommands far away from definition.

2 years ago

zuul commented 2 years ago

Build succeeded.

pre-commit : SUCCESS in 1m 58s
pytest-rawhide : SUCCESS in 2m 23s
pytest-f34 : SUCCESS in 2m 17s
pytest-f33 : SUCCESS in 2m 24s
rpm-mock-build : SUCCESS in 6m 22s

qulogic commented 2 years ago

I added tests and fixed a few bugs.

One question I found when writing tests: both changes and committing are enabled by default, but release/changelog might be skipped (with a log) if one's done already, just in case you've have-completed the change. But that also means you might be totally converted and it'll ignore that and make an empty conversion commit. Should it do that, or skip the commit?

zbyszek commented on line 19 of rpmautospec/subcommands/convert.py 2 years ago

RuntimeError(f"Spec file or path '{spec_or_path}' doesn't exist.")

The error string should be without "." to match the error messages in built-in exceptions. (Here and below.)

zbyszek commented on line 26 of rpmautospec/subcommands/convert.py 2 years ago

raise ValueError(f"Spec file '{spec_or_path}' must have '.spec' as an extension.")

Maybe Spec file {spec_or_path!r} must … ? This will handle the case where the string has a quote more explicitly.

zbyszek commented on line 169 of rpmautospec/subcommands/convert.py 2 years ago

default="Convert to rpmautospec.",

Please drop the dot here. (E.g. https://chris.beams.io/posts/git-commit/#seven-rules documents this very well.)

qulogic commented 2 years ago

RuntimeError(f"Spec file or path '{spec_or_path}' doesn't exist.")

The error string should be without "." to match the error messages in built-in exceptions. (Here and below.)

But it would be inconsistent with the rest of the code base.

raise ValueError(f"Spec file '{spec_or_path}' must have '.spec' as an extension.")

Maybe Spec file {spec_or_path!r} must … ? This will handle the case where the string has a quote more explicitly.

That would use the repr of a Path, not a string, but it could be done.

zbyszek commented 2 years ago

RuntimeError(f"Spec file or path '{spec_or_path}' doesn't exist.")

The error string should be without "." to match the error messages in built-in exceptions. (Here and below.)

But it would be inconsistent with the rest of the code base.

Nah, the surrounding code was already inconsistent. Let's at least do the correct thing in new code.

raise ValueError(f"Spec file '{spec_or_path}' must have '.spec' as an extension.")

Maybe Spec file {spec_or_path!r} must … ? This will handle the case where the string has a quote more explicitly.

That would use the repr of a Path, not a string, but it could be done.

Right, I didn't think about Path. Doesn't seem to be worth the trouble then.

rebased onto a635d5a

2 years ago

zuul commented 2 years ago

Build succeeded.

pre-commit : SUCCESS in 2m 27s
pytest-rawhide : SUCCESS in 3m 33s
pytest-f34 : SUCCESS in 3m 05s
pytest-f33 : SUCCESS in 2m 41s
rpm-mock-build : SUCCESS in 6m 30s

qulogic commented 2 years ago

raise ValueError(f"Spec file '{spec_or_path}' must have '.spec' as an extension.")

Maybe Spec file {spec_or_path!r} must … ? This will handle the case where the string has a quote more explicitly.

That would use the repr of a Path, not a string, but it could be done.

Right, I didn't think about Path. Doesn't seem to be worth the trouble then.

Ah, too late, I've already changed them.

zbyszek commented 2 years ago

Is anything else needed to get this merged? The command works great in the current form.

qulogic commented 2 years ago

Hello? It's been two months without comment, and this has been working fine.

nphilipp commented 2 years ago

Sorry for not getting back to you earlier… I've been tied up with other projects.

nphilipp commented 2 years ago

One of your commits wasn't signed that's why I couldn't merge. I took the liberty of fixing this locally as well as touching up commit messages so the subjects fit and the corresponding issue #201 is mentioned to be fixed, and merged that.

Thank you very much for contributing, and sorry again for the delay!