From 37b1e5ca472e9679301fa96d2b7fdd8c4ad438b2 Mon Sep 17 00:00:00 2001 From: Rhet Turnbull Date: Tue, 8 Dec 2020 08:32:37 -0800 Subject: [PATCH] Refactoring of save-config/load-config code --- osxphotos/__main__.py | 119 ++++++++-------- osxphotos/configoptions.py | 284 +++++++------------------------------ tests/test_cli.py | 25 +--- 3 files changed, 112 insertions(+), 316 deletions(-) diff --git a/osxphotos/__main__.py b/osxphotos/__main__.py index 7bc74941..44198921 100644 --- a/osxphotos/__main__.py +++ b/osxphotos/__main__.py @@ -25,7 +25,11 @@ from ._constants import ( UNICODE_FORMAT, ) from ._version import __version__ -from .configoptions import ExportOptions, InvalidOptions +from .configoptions import ( + ConfigOptions, + ConfigOptionsInvalidError, + ConfigOptionsLoadError, +) from .datetime_formatter import DateTimeFormatter from .exiftool import get_exiftool_path from .export_db import ExportDB, ExportDBInMemory @@ -1572,12 +1576,15 @@ def export( See --skip-edited, --skip-live, --skip-bursts, and --skip-raw options to modify this behavior. """ - - # NOTE: because of the way ExportOptions works, Click options must not + + # NOTE: because of the way ConfigOptions works, Click options must not # set defaults which are not None or False. If defaults need to be set # do so below after load_config and save_config are handled. - - cfg = ExportOptions(**locals()) + cfg = ConfigOptions( + "export", + locals(), + ignore=["ctx", "cli_obj", "dest", "load_config", "save_config"], + ) # print(jpeg_quality, edited_suffix, original_suffix) @@ -1585,11 +1592,14 @@ def export( VERBOSE = bool(verbose) if load_config: - cfg, error = ExportOptions().load_from_file(load_config, cfg) - # print(cfg.asdict()) - if error: - click.echo(f"Error parsing {load_config} config file: {error}", err=True) + try: + cfg.load_from_file(load_config) + except ConfigOptionsLoadError as e: + click.echo( + f"Error parsing {load_config} config file: {e.message}", err=True + ) raise click.Abort() + # re-set the local function vars to the corresponding config value # this isn't elegant but avoids having to rewrite this function to use cfg.varname for every parameter db = cfg.db @@ -1679,15 +1689,48 @@ def export( use_photokit = cfg.use_photokit report = cfg.report cleanup = cfg.cleanup - + # config file might have changed verbose VERBOSE = bool(verbose) verbose_(f"Loaded options from file {load_config}") + exclusive_options = [ + ("favorite", "not_favorite"), + ("hidden", "not_hidden"), + ("title", "no_title"), + ("description", "no_description"), + ("only_photos", "only_movies"), + ("burst", "not_burst"), + ("live", "not_live"), + ("portrait", "not_portrait"), + ("screenshot", "not_screenshot"), + ("slow_mo", "not_slow_mo"), + ("time_lapse", "not_time_lapse"), + ("hdr", "not_hdr"), + ("selfie", "not_selfie"), + ("panorama", "not_panorama"), + ("export_by_date", "directory"), + ("export_as_hardlink", "exiftool"), + ("place", "no_place"), + ("deleted", "deleted_only"), + ("skip_edited", "skip_original_if_edited"), + ("export_as_hardlink", "convert_to_jpeg"), + ("export_as_hardlink", "download_missing"), + ("shared", "not_shared"), + ("has_comment", "no_comment"), + ("has_likes", "no_likes"), + ] try: - cfg.validate(cli=True) - except InvalidOptions as e: - click.echo(f"{e.message}") + cfg.validate(exclusive_options, cli=True) + except ConfigOptionsInvalidError as e: + click.echo(f"Incompatible export options: {e.message}", err=True) + raise click.Abort() + + if missing and not download_missing: + click.echo( + "Incompatible export options: --missing must be used with --download-missing", + err=True, + ) raise click.Abort() if save_config: @@ -1697,7 +1740,9 @@ def export( # set defaults for options that need them jpeg_quality = DEFAULT_JPEG_QUALITY if jpeg_quality is None else jpeg_quality edited_suffix = DEFAULT_EDITED_SUFFIX if edited_suffix is None else edited_suffix - original_suffix = DEFAULT_ORIGINAL_SUFFIX if original_suffix is None else original_suffix + original_suffix = ( + DEFAULT_ORIGINAL_SUFFIX if original_suffix is None else original_suffix + ) # print(jpeg_quality, edited_suffix, original_suffix) @@ -1711,52 +1756,6 @@ def export( click.echo(f"report is a directory, must be file name", err=True) raise click.Abort() - # sanity check input args - exclusive = [ - (favorite, not_favorite), - (hidden, not_hidden), - (any(title), no_title), - (any(description), no_description), - (only_photos, only_movies), - (burst, not_burst), - (live, not_live), - (portrait, not_portrait), - (screenshot, not_screenshot), - (slow_mo, not_slow_mo), - (time_lapse, not_time_lapse), - (hdr, not_hdr), - (selfie, not_selfie), - (panorama, not_panorama), - (export_by_date, directory), - (export_as_hardlink, exiftool), - (any(place), no_place), - (deleted, deleted_only), - (skip_edited, skip_original_if_edited), - (export_as_hardlink, convert_to_jpeg), - (shared, not_shared), - (has_comment, no_comment), - (has_likes, no_likes), - (export_as_hardlink, cleanup), - ] - if any(all(bb) for bb in exclusive): - click.echo("Incompatible export options", err=True) - click.echo(cli.commands["export"].get_help(ctx), err=True) - return - - if export_as_hardlink and download_missing: - click.echo( - "Incompatible export options: --export-as-hardlink is not compatible with --download-missing", - err=True, - ) - raise click.Abort() - - if missing and not download_missing: - click.echo( - "Incompatible export options: --missing must be used with --download-missing", - err=True, - ) - raise click.Abort() - # if use_photokit and not check_photokit_authorization(): # click.echo( # "Requesting access to use your Photos library. Click 'OK' on the dialog box to grant access." diff --git a/osxphotos/configoptions.py b/osxphotos/configoptions.py index 611d4213..3afb4ecd 100644 --- a/osxphotos/configoptions.py +++ b/osxphotos/configoptions.py @@ -1,8 +1,8 @@ -""" Classes to load/save config settings for osxphotos CLI """ +""" ConfigOptions class to load/save config settings for osxphotos CLI """ import toml -class InvalidOptions(Exception): +class ConfigOptionsException(Exception): """ Invalid combination of options. """ def __init__(self, message): @@ -10,16 +10,31 @@ class InvalidOptions(Exception): super().__init__(self.message) -class OSXPhotosOptions: +class ConfigOptionsInvalidError(ConfigOptionsException): + pass + + +class ConfigOptionsLoadError(ConfigOptionsException): + pass + + +class ConfigOptions: """ data class to store and load options for osxphotos commands """ - def __init__(self, **kwargs): - args = locals() + def __init__(self, name, attrs, ignore=None): + """ init ConfigOptions class - self._attrs = {} - self._exclusive = [] + Args: + name: name for these options, will be used for section heading in TOML file when saving/loading from file + attrs: dict with name and default value for all allowed attributes + """ + self._name = name + self._attrs = attrs.copy() + if ignore: + for attrname in ignore: + self._attrs.pop(attrname, None) - self.set_attributes(args) + self.set_attributes(attrs) def set_attributes(self, args): for attr in self._attrs: @@ -27,7 +42,7 @@ class OSXPhotosOptions: arg = args[attr] # don't test 'not arg'; need to handle empty strings as valid values if arg is None or arg == False: - if self._attrs[attr] == (): + if type(self._attrs[attr]) == tuple: setattr(self, attr, ()) else: setattr(self, attr, self._attrs[attr]) @@ -36,7 +51,7 @@ class OSXPhotosOptions: except KeyError: raise KeyError(f"Missing argument: {attr}") - def validate(self, cli=False): + def validate(self, exclusive, cli=False): """ validate combinations of otions Args: @@ -49,14 +64,17 @@ class OSXPhotosOptions: InvalidOption if any combination of options is invalid InvalidOption.message will be descriptive message of invalid options """ + if not exclusive: + return True + prefix = "--" if cli else "" - for opt_pair in self._exclusive: + for opt_pair in exclusive: val0 = getattr(self, opt_pair[0]) val1 = getattr(self, opt_pair[1]) val0 = any(val0) if self._attrs[opt_pair[0]] == () else val0 val1 = any(val1) if self._attrs[opt_pair[1]] == () else val1 if val0 and val1: - raise InvalidOptions( + raise ConfigOptionsInvalidError( f"{prefix}{opt_pair[0]} and {prefix}{opt_pair[1]} options cannot be used together" ) return True @@ -78,234 +96,36 @@ class OSXPhotosOptions: data[attr] = val with open(filename, "w") as fd: - toml.dump({"export": data}, fd) + toml.dump({self._name: data}, fd) - def load_from_file(self, filename, override=None): + def load_from_file(self, filename, override=False): """ Load options from a TOML file. Args: filename: full path to TOML file - override: optional ExportOptions object; - if provided, any value that's set in override will be used - to override what's in the TOML file - - Returns: - (ExportOptions, error): tuple of ExportOption object and error string; - if there are any errors during the parsing of the TOML file, error will be set - to a descriptive error message otherwise it will be None - """ - override = override or ExportOptions() - loaded = toml.load(filename) - options = ExportOptions() - if "export" not in loaded: - return options, f"[export] section missing from {filename}" + override: bool; if True, values in the TOML file will override values already set in the instance - for attr in loaded["export"]: + Raises: + ConfigOptionsLoadError if there are any errors during the parsing of the TOML file + """ + loaded = toml.load(filename) + name = self._name + if name not in loaded: + raise ConfigOptionsLoadError(f"[{name}] section missing from {filename}") + + for attr in loaded[name]: if attr not in self._attrs: - return options, f"Unknown option: {attr}: {loaded['export'][attr]}" - val = loaded["export"][attr] - val = getattr(override, attr) or val - if self._attrs[attr] == (): + raise ConfigOptionsLoadError( + f"Unknown option: {attr} = {loaded[name][attr]}" + ) + val = loaded[name][attr] + if not override: + # use value from self if set + val = getattr(self, attr) or val + if type(self._attrs[attr]) == tuple: val = tuple(val) - setattr(options, attr, val) - return options, None + setattr(self, attr, val) + return self, None def asdict(self): return {attr: getattr(self, attr) for attr in sorted(self._attrs.keys())} - - -class ExportOptions(OSXPhotosOptions): - """ data class to store and load options for export command """ - - def __init__( - self, - db=None, - photos_library=None, - keyword=None, - person=None, - album=None, - folder=None, - uuid=None, - uuid_from_file=None, - title=None, - no_title=False, - description=None, - no_description=False, - uti=None, - ignore_case=False, - edited=False, - external_edit=False, - favorite=False, - not_favorite=False, - hidden=False, - not_hidden=False, - shared=False, - not_shared=False, - from_date=None, - to_date=None, - verbose=False, - missing=False, - update=True, - dry_run=False, - export_as_hardlink=False, - touch_file=False, - overwrite=False, - export_by_date=False, - skip_edited=False, - skip_original_if_edited=False, - skip_bursts=False, - skip_live=False, - skip_raw=False, - person_keyword=False, - album_keyword=False, - keyword_template=None, - description_template=None, - current_name=False, - convert_to_jpeg=False, - jpeg_quality=None, - sidecar=None, - only_photos=False, - only_movies=False, - burst=False, - not_burst=False, - live=False, - not_live=False, - download_missing=False, - exiftool=False, - ignore_date_modified=False, - portrait=False, - not_portrait=False, - screenshot=False, - not_screenshot=False, - slow_mo=False, - not_slow_mo=False, - time_lapse=False, - not_time_lapse=False, - hdr=False, - not_hdr=False, - selfie=False, - not_selfie=False, - panorama=False, - not_panorama=False, - has_raw=False, - directory=None, - filename_template=None, - edited_suffix=None, - original_suffix=None, - place=None, - no_place=False, - has_comment=False, - no_comment=False, - has_likes=False, - no_likes=False, - no_extended_attributes=False, - label=None, - deleted=False, - deleted_only=False, - use_photos_export=False, - use_photokit=False, - report=None, - cleanup=False, - **kwargs, - ): - args = locals() - - # valid attributes and default values - self._attrs = { - "db": None, - "photos_library": (), - "keyword": (), - "person": (), - "album": (), - "folder": (), - "uuid": (), - "uuid_from_file": None, - "title": (), - "no_title": False, - "description": (), - "no_description": False, - "uti": None, - "ignore_case": False, - "edited": False, - "external_edit": False, - "favorite": False, - "not_favorite": False, - "hidden": False, - "not_hidden": False, - "shared": False, - "not_shared": False, - "from_date": None, - "to_date": None, - "verbose": False, - "missing": False, - "update": False, - "dry_run": False, - "export_as_hardlink": False, - "touch_file": False, - "overwrite": False, - "export_by_date": False, - "skip_edited": False, - "skip_original_if_edited": False, - "skip_bursts": False, - "skip_live": False, - "skip_raw": False, - "person_keyword": False, - "album_keyword": False, - "keyword_template": (), - "description_template": None, - "current_name": False, - "convert_to_jpeg": False, - "jpeg_quality": None, - "sidecar": (), - "only_photos": False, - "only_movies": False, - "burst": False, - "not_burst": False, - "live": False, - "not_live": False, - "download_missing": False, - "exiftool": False, - "ignore_date_modified": False, - "portrait": False, - "not_portrait": False, - "screenshot": False, - "not_screenshot": False, - "slow_mo": False, - "not_slow_mo": False, - "time_lapse": False, - "not_time_lapse": False, - "hdr": False, - "not_hdr": False, - "selfie": False, - "not_selfie": False, - "panorama": False, - "not_panorama": False, - "has_raw": False, - "directory": None, - "filename_template": None, - "edited_suffix": None, - "original_suffix": None, - "place": (), - "no_place": False, - "has_comment": False, - "no_comment": False, - "has_likes": False, - "no_likes": False, - "no_extended_attributes": False, - "label": (), - "deleted": False, - "deleted_only": False, - "use_photos_export": False, - "use_photokit": False, - "report": None, - "cleanup": False, - } - - self._exclusive = [ - ["favorite", "not_favorite"], - ["hidden", "not_hidden"], - ["title", "no_title"], - ["description", "no_description"], - ] - - self.set_attributes(args) diff --git a/tests/test_cli.py b/tests/test_cli.py index 10871d0a..a2db68c7 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -875,7 +875,7 @@ def test_export_using_hardlinks_incompat_options(): "-V", ], ) - assert result.exit_code == 0 + assert result.exit_code == 1 assert "Incompatible export options" in result.output @@ -3961,26 +3961,3 @@ def test_export_cleanup(): assert not pathlib.Path("./delete_me.txt").is_file() assert not pathlib.Path("./foo/delete_me_too.txt").is_file() - -def test_export_cleanup_export_as_hardling(): - """ test export with incompatible option """ - import os - import os.path - from osxphotos.__main__ import export - - runner = CliRunner() - cwd = os.getcwd() - # pylint: disable=not-context-manager - with runner.isolated_filesystem(): - result = runner.invoke( - export, - [ - os.path.join(cwd, CLI_PHOTOS_DB), - ".", - "-V", - "--export-as-hardlink", - "--cleanup", - ], - ) - assert "Incompatible export options" in result.output -