Repository navigation
Miscellaneous minor refactoring of the unit formatters - #4001
Conversation
|
Getting a bunch of mysterious file not closed errors that didn't occur before... |
|
That lazy classproperty is a nice addition and clearly makes writing singletons much easier. These have always been singletons in concept, if not strictly speaking -- I was using instantiation as a trigger to generate the parsers the first time, but clearly these lazy classproperties are much nicer. Be sure to put that in CHANGES as a separate item under utils -- I think that's one of the nice side effects of this PR. |
|
@mdboom The classproperty decorator is new as of a couple PRs ago anyways, and are already mentioned in the changelog. I just added the "lazy" feature after realizing the use for this change. Does still need a changelog entry to emphasize the change in formatters. |
|
Also glad they were always intended as singletons--I was trying to think if maybe there could be some case for some kind of instantiated formatter, but couldn't think of anything immediately. |
It's a minor point. I've always considered any thing in |
There was a problem hiding this comment.
These ones don't really have to be lazy at all, right? I'd think only _all_units would need to be calculated only once.
There was a problem hiding this comment.
No, not strictly necessary. It doesn't matter much either way in this case.
|
@embray - nice indeed! |
… sake of consistency (about half were classmethod and half were instancemethods--in all cases a classmethod was fine)
… lazy evaluation of attributes a la lazyproperty
…normal methods, and in some other cases they were classmethods, and the inconsistency irked me. This also makes sure that the parse and to_string classmethods will always work even if no instance has been made of the formatter, by making use of the new classproperty decorator. At this point the Formatter classes are true singletons (maybe an example could be found where we wouldn't want singletons, but of the existing Formatters there's no reason any of them need to be instantiated)
…new__ that makes instantiation completely superfluous.
…but valid, according to the language definition) hacks to get around circular import issues.
e39936e to
65a5319
Compare
|
For some reason this wasn't re-picked up by CI watchers when I rebased. Trying again.... |
65a5319 to
620ee44
Compare
Miscellaneous minor refactoring of the unit formatters
3 participants
Most of these changes were driven by the observation I made, upon going to make some changes for #3986, that on some of the formatter classes (including the
Baseclass) theto_stringmethod was a normal instance method, while on others (about half) it was aclassmethod. This incongruity bugged me.After looking over the other classes I decided it would be possible to make the
parseandto_stringmethods on all the formatter classes intoclassmethods, so that now each formatter class is basically acting as a singleton (it would actually be a good use case for a general Singleton type, but I decided to leave that as an exercise for another time).I think there are probably others bits of repetition between the formatter implementations, but I think that's about all I feel like addressing for now.
This also removed all inline imports from
parseandto_stringmethods. A bit of a hack, but doable. Between eliminating formatter instantiation and inline imports, the time to instantiate aUnitfrom a string with the generic formatter (after already using the formatter at least once, so parser/lexer setup is excluded from this timing) went from about 13.1µs to about 9.2µs, or about 30%. Not super-dramatic, but still worthwhile.