Skip to content

Commit

Permalink
Remove check- prefix from subcommands of promoted profiles
Browse files Browse the repository at this point in the history
  • Loading branch information
@miguelsousa
miguelsousa committed Jun 23, 2023
1 parent d6600a7 commit 489a2cc
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 107 deletions.
19 changes: 13 additions & 6 deletions .github/workflows/install_run.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,30 @@ jobs:
- name: Run Universal checks
run: >-
openbakery check-universal
openbakery universal
-x win_ascent_and_descent
-x os2_metrics_match_hhea
-x soft_dotted
data/test/source-sans-pro/OTF/SourceSansPro-Regular.otf
data/test/source-sans-pro/OTF/SourceSansPro-Italic.otf;
openbakery check-universal
openbakery universal
-x win_ascent_and_descent
-x os2_metrics_match_hhea
-x fsselection
-x valid_default_instance_nameids
-x soft_dotted
data/test/source-sans-pro/VAR/SourceSansVariable-Roman.ttf
- name: Run Universal check using 'check-profile'
run: >-
openbakery check-profile openbakery.profiles.universal
data/test/mada/Mada-Regular.ttf
--verbose
-c required_tables
- name: Try running UFO Sources checks
run: >-
openbakery check-ufo-sources --verbose data/test/test.ufo
openbakery ufo-sources --verbose data/test/test.ufo
|| exit 0
shell: bash

Expand All @@ -71,8 +78,8 @@ jobs:
- name: Run UFO Sources checks
run: >-
openbakery check-ufo-sources --verbose data/test/test.ufo;
openbakery check-ufo-sources -x designspace_has_consistent
openbakery ufo-sources --verbose data/test/test.ufo;
openbakery ufo-sources -x designspace_has_consistent
"data/test/stupidfont/Stupid Font.designspace"
- name: Install `googlefonts` extra
Expand All @@ -81,7 +88,7 @@ jobs:
- name: Run Google Fonts checks
run: >-
openbakery check-googlefonts
openbakery googlefonts
-c canonical_filename
-c vendor_id
-c glyph_coverage
Expand Down
57 changes: 22 additions & 35 deletions Lib/openbakery/cli.py
View file
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import argparse
import pkgutil
import runpy
import signal
import sys
from importlib import import_module

import openbakery.commands
from openbakery import __version__
from openbakery.commands.check_profile import main as check_profile_main

CLI_PROFILES = [
SUBCOMMANDS = [
"adobefonts",
"fontbureau",
"fontval",
Expand All @@ -17,14 +15,15 @@
"iso15008",
"notofonts",
"opentype",
"ufo_sources",
"ufo-sources",
"universal",
"proposals",
"check-profile",
]


def run_profile_check(profilename):
module = import_module(f"openbakery.profiles.{profilename}")
module = import_module(f"openbakery.profiles.{profilename.replace('-', '_')}")
sys.exit(check_profile_main(module.profile))


Expand All @@ -36,54 +35,42 @@ def signal_handler(sig, frame):
def main():
signal.signal(signal.SIGINT, signal_handler)

subcommands = [
pkg[1] for pkg in pkgutil.walk_packages(openbakery.commands.__path__)
] + ["check_" + prof for prof in CLI_PROFILES]

subcommands = [command.replace("_", "-") for command in sorted(subcommands)]

if len(sys.argv) >= 2 and sys.argv[1] in subcommands:
# Relay to subcommand.
if len(sys.argv) >= 2 and sys.argv[1] in SUBCOMMANDS:
subcommand = sys.argv[1]
subcommand_module = subcommand.replace("-", "_")

sys.argv[0] += " " + subcommand
del sys.argv[1] # Make this indirection less visible for subcommands.
if (
subcommand_module.startswith("check_")
and subcommand_module[6:] in CLI_PROFILES
):
run_profile_check(subcommand_module[6:])

if subcommand == "check-profile":
check_profile_main()
else:
runpy.run_module(
"openbakery.commands." + subcommand_module, run_name="__main__"
)
run_profile_check(subcommand)

else:
description = (
"Run openbakery subcommands. Subcommands have their own help "
"messages. These are usually accessible with the -h/--help flag "
"positioned after the subcommand, i.e.: openbakery subcommand -h"
"Run openbakery subcommands. Subcommands have their own help messages;\n"
"to view them add the '-h' (or '--help') option after the subcommand,\n"
"like in this example:\n openbakery universal -h"
)

parser = argparse.ArgumentParser(description=description)
parser = argparse.ArgumentParser(
formatter_class=argparse.RawTextHelpFormatter, description=description
)
parser.add_argument(
"subcommand",
help="The subcommand to execute",
nargs="?",
choices=subcommands,
choices=SUBCOMMANDS,
)
parser.add_argument(
"--list-subcommands",
action="store_true",
help="print the list of subcommands "
"to stdout, separated by a space character. This is "
"usually only used to generate the shell completion code.",
)
parser.add_argument(
"--version", action="version", version=openbakery.__version__
help="print list of supported subcommands",
)
parser.add_argument("--version", action="version", version=__version__)
args = parser.parse_args()

if args.list_subcommands:
print(" ".join(subcommands))
print("\n".join(SUBCOMMANDS))
else:
parser.print_help()
14 changes: 7 additions & 7 deletions Lib/openbakery/commands/check_profile.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def ArgumentParser(profile, profile_arg=True):

if profile_arg:
argument_parser.add_argument(
"profile", help="File/Module name," ' must define a openbakery "profile".'
"profile", help="File/Module name, must define an openbakery 'profile'."
)

values_keys = profile.setup_argparse(argument_parser)
Expand All @@ -78,7 +78,7 @@ def ArgumentParser(profile, profile_arg=True):
"--checkid",
action="append",
help=(
"Explicit check-ids (or parts of their name) to be executed. "
"Explicit check-ids (or parts of their name) to be executed.\n"
"Use this option multiple times to select multiple checks."
),
)
Expand All @@ -88,7 +88,7 @@ def ArgumentParser(profile, profile_arg=True):
"--exclude-checkid",
action="append",
help=(
"Exclude check-ids (or parts of their name) from execution. "
"Exclude check-ids (or parts of their name) from execution.\n"
"Use this option multiple times to exclude multiple checks."
),
)
Expand All @@ -106,7 +106,7 @@ def log_levels_get(key):
dest="loglevels",
const=PASS,
action="append_const",
help="Shortcut for `-l PASS`.\n",
help="Shortcut for '-l PASS'.\n",
)

argument_parser.add_argument(
Expand Down Expand Up @@ -135,23 +135,23 @@ def log_levels_get(key):
argument_parser.add_argument(
"--succinct",
action="store_true",
help="This is a slightly more compact and succint" " output layout.",
help="This is a slightly more compact and succint output layout.",
)

argument_parser.add_argument(
"-n",
"--no-progress",
default=False,
action="store_true",
help="In a tty as stdout, don't" " render the progress indicators.",
help="Suppress the progress indicators in the console output.",
)

argument_parser.add_argument(
"-C",
"--no-colors",
default=False,
action="store_true",
help="No colors for tty output.",
help="Suppress the coloring theme in the console output.",
)

argument_parser.add_argument(
Expand Down
96 changes: 37 additions & 59 deletions tests/commands/test_usage.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,45 +5,49 @@

import pytest

TOOL_NAME = "openbakery"

def test_list_subcommands_has_all_scripts():
"""Tests if the output from running `openbakery --list-subcommands` matches
the openbakery scripts within the bin folder and the promoted profiles."""
import openbakery.commands
from openbakery.cli import CLI_PROFILES

commands_dir = os.path.dirname(openbakery.commands.__file__)

scripts = [
f.rstrip(".py").replace("_", "-")
for f in os.listdir(commands_dir)
if (f.endswith(".py") and not f.startswith("_"))
]
scripts = scripts + [("check-" + i).replace("_", "-") for i in CLI_PROFILES]
subcommands = (
subprocess.check_output(["openbakery", "--list-subcommands"]).decode().split()
)
assert sorted(scripts) == sorted(subcommands)

def test_list_subcommands_option(capfd):
"""Test if 'openbakery --list-subcommands' can run successfully and output
the expected content."""
from openbakery.cli import SUBCOMMANDS

def test_command_check_googlefonts():
"""Test if `openbakery check-googlefonts` can run successfully`."""
subprocess.check_output(["openbakery", "check-googlefonts", "-h"])
subprocess.check_call([TOOL_NAME, "--list-subcommands"])
captured = capfd.readouterr()
assert captured.out == "\n".join(SUBCOMMANDS) + "\n"

test_font = os.path.join("data", "test", "nunito", "Nunito-Regular.ttf")

subprocess.check_output(
def test_command_check_googlefonts():
"""Test if 'openbakery googlefonts' can run successfully."""
subprocess.check_call([TOOL_NAME, "googlefonts", "-h"])
subprocess.check_call(
[
"openbakery",
"check-googlefonts",
TOOL_NAME,
"googlefonts",
"-c",
"com.google.fonts/check/canonical_filename",
test_font,
os.path.join("data", "test", "nunito", "Nunito-Regular.ttf"),
]
)
with pytest.raises(subprocess.CalledProcessError):
subprocess.check_call([TOOL_NAME, "googlefonts"])


@pytest.mark.parametrize(
"subcommand",
[
"check-profile",
"opentype",
"ufo-sources",
],
)
def test_command_check_profile(subcommand):
"""Test if 'openbakery <subcommand>' can run successfully."""
subprocess.check_call([TOOL_NAME, subcommand, "-h"])

with pytest.raises(subprocess.CalledProcessError):
subprocess.check_output(["openbakery", "check-googlefonts"])
subprocess.check_call([TOOL_NAME, subcommand])


@pytest.mark.xfail(
Expand All @@ -54,17 +58,15 @@ def test_command_check_googlefonts():
# Please, only remove the xfail mark once the test is more robust / future proof.
def test_status_log_is_indented():
"""Test if statuses are printed in a limited boundary."""
test_font = os.path.join("data", "test", "nunito", "Nunito-Regular.ttf")

result = subprocess.run(
[
"openbakery",
"check-googlefonts",
TOOL_NAME,
"googlefonts",
"-c",
"old_ttfautohint",
"-c",
"font_copyright",
test_font,
os.path.join("data", "test", "nunito", "Nunito-Regular.ttf"),
],
capture_output=True,
)
Expand Down Expand Up @@ -92,38 +94,14 @@ def test_status_log_is_indented():
)


def test_command_check_profile():
"""Test if `openbakery check-profile` can run successfully`."""
subprocess.check_output(["openbakery", "check-profile", "-h"])

with pytest.raises(subprocess.CalledProcessError):
subprocess.check_output(["openbakery", "check-profile"])


def test_command_check_opentype():
"""Test if `openbakery check-opentype` can run successfully`."""
subprocess.check_output(["openbakery", "check-opentype", "-h"])

with pytest.raises(subprocess.CalledProcessError):
subprocess.check_output(["openbakery", "check-opentype"])


def test_command_check_ufo_sources():
"""Test if `openbakery check-ufo-sources` can run successfully`."""
subprocess.check_output(["openbakery", "check-ufo-sources", "-h"])

with pytest.raises(subprocess.CalledProcessError):
subprocess.check_output(["openbakery", "check-ufo-sources"])


def test_command_config_file():
"""Test if we can set checks using a config file."""
config = tempfile.NamedTemporaryFile(delete=False)
config.write(b"explicit_checks = ['com.adobe.fonts/check/name/empty_records']")
config.close()
test_font = os.path.join("data", "test", "nunito", "Nunito-Regular.ttf")
result = subprocess.run(
["openbakery", "check-googlefonts", "--config", config.name, test_font],
[TOOL_NAME, "googlefonts", "--config", config.name, test_font],
stdout=subprocess.PIPE,
)
stdout = result.stdout.decode()
Expand All @@ -145,7 +123,7 @@ def test_command_config_file_injection():
test_profile = os.path.join("tests", "profiles", "a_test_profile.py")
result = subprocess.run(
[
"openbakery",
TOOL_NAME,
"check-profile",
"-C",
"--config",
Expand Down Expand Up @@ -175,7 +153,7 @@ def test_config_override():
config.close()
test_font = os.path.join("data", "test", "varfont", "inter", "Inter[slnt,wght].ttf")
result = subprocess.run(
["openbakery", "check-googlefonts", "-C", "--config", config.name, test_font],
[TOOL_NAME, "googlefonts", "-C", "--config", config.name, test_font],
stdout=subprocess.PIPE,
)
stdout = result.stdout.decode()
Expand Down

10 comments on commit 489a2cc

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

No, please don't do it!

There's a plan to add fixing features (which we had in the past but temporarily removed to be more focused on checking). In that case we may have commands like fontbakery fix-googlefonts or something similar.

Please ask the user/dev community before this kind of change to the tool.

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what is the meaning of a "promoted" profile?

@ftCLI
Copy link

@ftCLI ftCLI commented on 489a2cc Jun 23, 2023

Choose a reason for hiding this comment

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

No, please don't do it!

There's a plan to add fixing features (which we had in the past but temporarily removed to be more focused on checking). In that case we may have commands like fontbakery fix-googlefonts or something similar.

Please ask the user/dev community before this kind of change to the tool.

Wouldn't be better introducing fixes for single issues instead that for the whole profile? For example, fontbakery fix-italic-angle instead of fontbakery fix-post.

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure! The specific implementation details are a matter yet to be discussed. In the past it was exactly as you proposed, by the way.

The point here is that the "check-something" commands are here to distinguish from other kinds of commands (such as "fix-something").

@ftCLI
Copy link

@ftCLI ftCLI commented on 489a2cc Jun 23, 2023

Choose a reason for hiding this comment

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

Good to know, but in my humble opinion fix should live out of font/openbakery.

@miguelsousa
Copy link
Owner Author

Choose a reason for hiding this comment

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

@felipesanches

I got the "promoted" term from here: https://github.com/googlefonts/fontbakery/blob/31bb1ca702e771c971d2d8d0b16f5c5f0a210c4d/tests/commands/test_usage.py#L11

I think it's the first time I'm hearing about the fix- idea. Thanks for mentioning it. At this point it's hard to say if that feature will ever be wanted or needed in OpenBakery, so until then it's better to keep things simple. You're welcome to submit a proposal.

Your input is welcomed, but you need to keep in mind that this project is not Font Bakery; it's a fork and as such it may take different development paths. Feedback from the community is important. But this project is very new, only has one developer, and I haven't seen any evidence that the openbakery tool is being used by anyone but me. I'm still working toward having a version 1.x release. The driving efforts until then are 1) reduce the project's default footprint, and 2) make the project more user- and developer-friendly. After the 1.x release any major changes will be proposed and discussed first. Keep an eye on https://github.com/miguelsousa/openbakery/discussions

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

I got the "promoted" term from here: https://github.com/googlefonts/fontbakery/blob/31bb1ca702e771c971d2d8d0b16f5c5f0a210c4d/tests/commands/test_usage.py#L11

It seems that that line was written by @simoncozens at fonttools/fontbakery@bd724c4
But I still don't know what he meant then - or what you mean now - by using that term. It would be useful to document its meaning explicitly.

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's the first time I'm hearing about the fix- idea.

fonttools/fontbakery#1501

@simoncozens
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant is that some of the files under Lib/fontbakery/profile (e.g. universal) are actually profiles in the sense of a list of checks that you want to use, but some of the files in that directory (e.g. post) are not actually top-level profiles but are simply implementations of checks. See #3188.

@felipesanches
Copy link
Contributor

Choose a reason for hiding this comment

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

See #3188.

fonttools/fontbakery#3188

Please sign in to comment.