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

fontsetplot and ttfdecomponentizer fixes #1148

Merged
merged 3 commits into from
Jun 1, 2020
Merged

fontsetplot and ttfdecomponentizer fixes #1148

merged 3 commits into from
Jun 1, 2020

Conversation

miguelsousa
Copy link
Member

Fixes problems raised in #1125.

Copy link
Collaborator

@josh-hadley josh-hadley left a comment

Choose a reason for hiding this comment

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

In ttfdecomponentizer: this still fails if you specify -d + an input directory, but no output with -o. It seems like we are implying that if you specify a directory as input, you must also explicitly specify -o (and it must be a directory). But we aren't really enforcing that in the options validation.

Example:

ttfdecomponentizer -d some/input/directory -o some/output/directory

works, but

ttfdecomponentizer -d some/input/directory

fails (arg validation succeeds, but subsequent operations raise an Exception because the output path is None)

N.B. it also failed before these changes, but since we're in here, I think we should address it. Possible ways to handle this case (-d with valid input, but -o not specified):

  1. set output directory == input. This means any files in the dir would get modified in-place, which is the default behavior of the tool in single-file mode
  2. raise arg parser error something like, "must explicitly set -o when using -d for input". This is slightly "safer" than option 1 because it would require a user to intentionally set output dir == input dir.

…on is used but -o is not

Plus minor editorial changes to the options.
@miguelsousa
Copy link
Member Author

@josh-hadley good catch! Option 1 seemed the best to me, so I went with that.

Copy link
Collaborator

@josh-hadley josh-hadley left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @miguelsousa !

@miguelsousa miguelsousa merged commit e69b531 into develop Jun 1, 2020
@miguelsousa miguelsousa deleted the ms-fixes branch June 1, 2020 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants