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

[filters] Add loadFilterFromString() #466

Merged
merged 7 commits into from
Feb 26, 2021

Conversation

moyogo
Copy link
Collaborator

@moyogo moyogo commented Feb 18, 2021

This add the ufo2ft.filters.loadFilterFromString() function to be used in fontmake.
See:

BaseFilter only takes kwargs instead of args and kwargs. None of the ufo2ft filters use the args.
Edit: The args can be given with kwargs key args.

@moyogo moyogo marked this pull request as ready for review February 25, 2021 13:34
Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

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

Thanks Denis for working on this.
I see this resembles very much the way we treat loading featureWriters from a string. I wonder if we can find a way to try reuse the shared logic in both instead of duplicating it? Like a generic _loadPluginFromString or something

@moyogo
Copy link
Collaborator Author

moyogo commented Feb 25, 2021

I see this resembles very much the way we treat loading featureWriters from a string. I wonder if we can find a way to try reuse the shared logic in both instead of duplicating it? Like a generic _loadPluginFromString or something

I added _loadPluginFromString().

raise ValueError("options have incorrect format: %r" % kwargs)

if hasattr(klass, "_args"):
# Process positional arguments
Copy link
Member

@anthrotype anthrotype Feb 26, 2021

Choose a reason for hiding this comment

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

this posargs handling could be moved to the base classes constructors, so they can also be initialized using keywords.. But then you'd have to duplicate in both BaseFeatureWriter and BaseFilter __init__.. I guess it's ok like that

Copy link
Member

Choose a reason for hiding this comment

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

actually, only the filters can have positional arguments at all (have that _args attribute that you're checking in here). Yeah, i think it makes more sense to move this logic to base filter class init. Do you mind doing that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

BaseFeatureWriter and BaseFilter should probably inherit from a common BasePlugin class. Should that be in the PR or another?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that'd be nice too. I'd say do it in same PR

Copy link
Member

Choose a reason for hiding this comment

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

if you can't reconcile the differences between the two base classes, then just move this pos-to-kwargs logic to the base filter init

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. I removed posargs logic from _loadPluginFromString and changed BaseFilter. That's much nicer.

Copy link
Member

Choose a reason for hiding this comment

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

it seems you're only handling the case where all the positionals are specified as keywords, see my other comment #466 (comment)

@moyogo
Copy link
Collaborator Author

moyogo commented Feb 26, 2021

why only 0? One could potentially want to construct a filter with some strictly positional and some positional-as-keywords.

This sounds like a bad idea. Now I have to check for duplicated args as both positional and positional-as-keywords.

@anthrotype
Copy link
Member

anthrotype commented Feb 26, 2021

This sounds like a bad idea.

but that's how python args and kwargs work, would be weird to diverge from that. Unless a parameter is marked as positional-only, one can always use the equivalent keyword to pass the argument

@moyogo moyogo merged commit 4f86374 into googlefonts:master Feb 26, 2021
@moyogo moyogo mentioned this pull request Feb 26, 2021
@moyogo moyogo deleted the add-loadFilterFromString branch March 2, 2021 06:39
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