-
Notifications
You must be signed in to change notification settings - Fork 43
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
Conversation
40e7962
to
1bf578a
Compare
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! |
it still adds another 789 ;) |
Good point, I missed a directory when counting. :-) |
14801e7
to
f45809e
Compare
can we make this new |
This reverts commit 2242439.
There was a problem hiding this 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!
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! (It's still a good idea.)compileVariableCFF2s