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

Add command line script #73

Merged
merged 4 commits into from
Sep 27, 2024
Merged

Add command line script #73

merged 4 commits into from
Sep 27, 2024

Conversation

rimas-kudelis
Copy link
Contributor

@rimas-kudelis rimas-kudelis commented Sep 26, 2024

This rebases and #35 from master and adds some small improvements over it, namely:

  • turns ufoLib2 from hard to an optional dependency
  • attempts to fall back to defcon if ufoLib2 is not installed
  • outputs status messages when running the script

Fixes #35, fixes #60 (although the script is still not as fancy as the one suggested in #60).

jenskutilek and others added 3 commits February 19, 2021 09:26
- make ufoLib2 an optional dependency
- attempt to fall back to defcon if ufoLib2 is not installed
- output status messages when running the script
@benkiel
Copy link
Member

benkiel commented Sep 26, 2024

Can we add an optional command line option to choose between ufoLib2 and defcon for those who have both installed? I know this is a small fussy detail, but it might be helpful for some

@rimas-kudelis
Copy link
Contributor Author

Ugh, tried to avoid messing with the args parser, but okay, I guess it's time to grow up. I'll try. 😄

@benkiel
Copy link
Member

benkiel commented Sep 26, 2024

@rimas-kudelis steal from here: #35 (comment)

@rimas-kudelis
Copy link
Contributor Author

Yep, I'm looking at it, but it won't be just stealing. That command requires that the input font is TTF/OTF, for example, and extractor supports more formats than just that. I'll add the output format argument from that script though.

I'm on it already.

@rimas-kudelis
Copy link
Contributor Author

@benkiel, done. I could still add even more options, like maybe one for the destination directory.

Comment on lines +81 to +86
"""
Extract one ore more fonts to UFO. Installed as command line script
`extractufo`.

Usage: extractufo font [font ...]
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, not sure if it makes sense to keep this comment now that we're using the argument parser.

@rimas-kudelis
Copy link
Contributor Author

rimas-kudelis commented Sep 26, 2024

Ouch, it seems defcon doesn't support the overwrite argument to save(). How should I handle this? Shall I just remove this option, or require to be present it when defcon is to be used as the backend?

Also, is there any quick kind of a test I could add, assuming I know very little about Python and pytest?

@benkiel
Copy link
Member

benkiel commented Sep 26, 2024

@rimas-kudelis Looking at the code for overwrite in ufoLib2, I would say we should just remove the overwrite option. If you want to keep it, would say "This will not work if backend is defcon" and then do a check for the backend, if it's defcon, do not pass overwrite to save.

)
parser.add_argument('FONT_FILE', help='Input font path', nargs="+")
parser.add_argument('-m', '--ufo-module', choices=['ufoLib2', 'defcon'], help='Select the default library for writing UFOs (default: autodetect, prefer ufoLib2)')
parser.add_argument('-z', '--zip', action="store_true", help="Output UFO ZIP")
Copy link
Member

Choose a reason for hiding this comment

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

help="Output UFOZ"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, "UFO ZIP" is actually how the specification calls it. :)

Copy link
Member

Choose a reason for hiding this comment

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

Cool. I can't remember, and I am used to thinking UFOZ, but fine with UFO ZIP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From https://unifiedfontobject.org/versions/ufo3/#storage-formats:

UFO has two possible storage formats. The first, “UFO Package”, is a multi-file, file system directory structure. The second, “UFO ZIP”, is a ZIP archive containing the same directory structure as defined for UFO Package.

I'll leave this as-is then.

ufo = Font()
extractUFO(font_path, ufo)
try:
ufo.save(ufo_path, overwrite=args.overwrite, structure=structure)
Copy link
Member

Choose a reason for hiding this comment

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

could just drop overwrite here for simplicity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I've removed the option, and added an explicit check for file/dir existence. If it exists, that input file will not be converted.
I reckon that's better than having two different scenarios, right?
Hm, maybe I should make a PR for defcon adding this option though... 😆

@benkiel benkiel merged commit 82a5a45 into robotools:master Sep 27, 2024
9 checks passed
@benkiel
Copy link
Member

benkiel commented Sep 27, 2024

Thank you! Going to push this out as a new release

@rimas-kudelis rimas-kudelis deleted the cmdline branch September 27, 2024 06:06
@rimas-kudelis
Copy link
Contributor Author

I think maybe we should update the readme first.

@NightFurySL2001
Copy link
Contributor

Thanks @rimas-kudelis for the awesome job! The parent folder part in my script is just to organise the files easier, in the end I end up not using that option at all 😂

@rimas-kudelis
Copy link
Contributor Author

@NightFurySL2001, thank you for your script as well! As you see, I took some inspiration from it. :)

BTW I had some doubts about whether I should accept an unlimited list of input files or instead accept only one input file and allow specifying output file name like you did, but I guess I was too lazy to change that bit. But I do think that maybe your approach is better, since looping could have easily be done externally in the shell. Oh well... :)

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.

Request to provide command line interface program
4 participants