Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IMP] allow fix_manifest_website.py to fix only specified manifest files #522

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions tests/test_fix_manifest_website.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,25 @@ def test_fix_manifest_website(tmp_path):
assert (
tmp_path / "a2" / "__manifest__.py"
).read_text() == """{'name': 'a2', "website" :\n "https://new.url"}"""


def test_fix_specific_manifest_website(tmp_path):
(tmp_path / "a1").mkdir()
(tmp_path / "a1" / "__manifest__.py").write_text(
"""{'name': 'a1', 'website': '...'}"""
)
(tmp_path / "a2").mkdir()
manifest_path_a2 = tmp_path / "a2" / "__manifest__.py"
manifest_path_a2.write_text(
"""{'name': 'a2', "website" :\n "https://bad.url"}"""
)
# Only manifest under a2 directory should be fixed
result = CliRunner().invoke(
main, ["--addons-dir", str(tmp_path), "https://new.url", str(manifest_path_a2)]
)
assert result.exit_code == 0
assert (
tmp_path / "a1" / "__manifest__.py"
).read_text() == """{'name': 'a1', 'website': '...'}"""
manifest_content = """{'name': 'a2', "website" :\n "https://new.url"}"""
assert manifest_path_a2.read_text() == manifest_content
78 changes: 48 additions & 30 deletions tools/fix_manifest_website.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Set the website key in addons manifests."""
import os
import re
from pathlib import Path

import click

Expand All @@ -12,36 +13,53 @@

@click.command()
@click.argument("url")
@click.argument("manifest", nargs=-1, type=click.Path())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@click.argument("manifest", nargs=-1, type=click.Path())
@click.argument("manifest", nargs=-1, type=click.Path(exists=True))

@click.option("--addons-dir", default=".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to have some help text saying that --addons-dir and manifests are mutually exclusive, and raise an error rather than ignoring silently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sbidoul your comments are helpful. I will address them later and ping you back.

def main(url, addons_dir):
for addon_dir in os.listdir(addons_dir):
manifest_path = get_manifest_path(os.path.join(addons_dir, addon_dir))
if not manifest_path:
continue
try:
with open(manifest_path) as manifest_file:
manifest = parse_manifest(manifest_file.read())
except Exception:
raise click.ClickException(
"Error parsing manifest {}.".format(manifest_path)
)
if "website" not in manifest:
raise click.ClickException(
"website key not found in manifest in {}.".format(addon_dir)
)
def main(url, addons_dir, manifest):
# Specifying one or more manifest files has the highest priority
if manifest:
for manifest_file in manifest:
_fix_website(Path(manifest_file).parent, manifest_file, url)
else:
for addon_dir in os.listdir(addons_dir):
manifest_path = get_manifest_path(os.path.join(addons_dir, addon_dir))
if not manifest_path:
continue
_fix_website(addon_dir, manifest_path, url)


def _fix_website(addon_dir, manifest_path, url):
"""
Replaces the website in the manifest pointed by manifest_path by url.

:param manifest_path: path to the manifest file, relative or absolute
:param addon_dir: used to indicate errors, points to parent path of manifest file
:param url: new value for the "website" key in the manifest content
"""
try:
with open(manifest_path) as manifest_file:
manifest_str = manifest_file.read()
new_manifest_str, n = WEBSITE_KEY_RE.subn(
r"\g<1>" + url + r"\g<3>", manifest_str
manifest = parse_manifest(manifest_file.read())
except Exception:
raise click.ClickException(
"Error parsing manifest {}.".format(manifest_path)
)
if "website" not in manifest:
raise click.ClickException(
"website key not found in manifest in {}.".format(addon_dir)
)
with open(manifest_path) as manifest_file:
manifest_str = manifest_file.read()
new_manifest_str, n = WEBSITE_KEY_RE.subn(
r"\g<1>" + url + r"\g<3>", manifest_str
)
if n == 0:
raise click.ClickException(
"no website key match in manifest in {}.".format(addon_dir)
)
if n > 1:
raise click.ClickException(
"more than one website key match in manifest in {}.".format(addon_dir)
)
if n == 0:
raise click.ClickException(
"no website key match in manifest in {}.".format(addon_dir)
)
if n > 1:
raise click.ClickException(
"more than one website key match in manifest in {}.".format(addon_dir)
)
if new_manifest_str != manifest_str:
with open(manifest_path, "w") as manifest_file:
manifest_file.write(new_manifest_str)
if new_manifest_str != manifest_str:
with open(manifest_path, "w") as manifest_file:
manifest_file.write(new_manifest_str)