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

Refactor into compilers #801

Merged
merged 18 commits into from
Dec 4, 2023
Merged

Refactor into compilers #801

merged 18 commits into from
Dec 4, 2023

Conversation

simoncozens
Copy link
Contributor

@simoncozens simoncozens commented Nov 29, 2023

This is a large refactor. We have, in init.py, a bunch of very similar functions which differ only slightly, mostly in terms of default parameters and small pieces of logic. This means (a) there's a lot of repetition, (b) there's a lot of horrible mangling of parameters, and (c) it's hard to see the small pieces which are different because of the repetition.

To mitigate this up to this point, we have essentially tried to re-create inheritance by having these functions call things like call_preprocessor, call_postprocessor (abstracting out what we can of the commonalities), and having ad hoc specialisation of the function arguments. In other words, we have tried to turn our functions into pseudo-classes. It turns out to be much neater to turn them into real classes instead.

Turning them into classes saves all the repetition, removes around 800 lines of code, makes it easier to actually find things, and possible also has exposed a bug in compileVariableCFF2s! (It's still a good idea.)

@anthrotype
Copy link
Member

anthrotype commented Nov 29, 2023

in general i'm supportive of this, as long as it doesn't unnecessarily breaks existing code that uses ufo2ft. Since these are lots of changes, it'd take me a while to review. But thanks Simon!

@anthrotype
Copy link
Member

removes around 800 lines

it still adds another 789 ;)

@simoncozens
Copy link
Contributor Author

Good point, I missed a directory when counting. :-)

@anthrotype
Copy link
Member

can we make this new compilers subpackage private (e.g. prefix with underscore _compilers)? I wouldn't like to expose these new classes but prefer we keep the existing interface.

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.

maybe we can stop fiddling with this and start merging, so the other PRs can be rebased off it, we can fix up remaining issues if any before release

thanks for patience!

@simoncozens simoncozens merged commit a421acc into main Dec 4, 2023
9 checks passed
@anthrotype anthrotype deleted the refactor-compilers branch December 4, 2023 15:09
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