Closed happytree718 closed 1 year ago
Patch coverage: 100.00
% and no project coverage change.
Comparison is base (
c0b1c77
) 100.00% compared to head (ea6e689
) 100.00%.
:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
The overall direction is correct. A couple of suggestions.
register
is a too generic name(I know I used it in the issue, but I did not mean to directy use it as the function name), consider register_formatter
.register_formatter
function for both cases:
type
is given, it's a decorator and should return a wrapper to decorate the functionhex
is a good example, but we can also add an example with lambda
, which could also be very common and useful.inherit=True
, which indicates whether this formatter would be inherited by subclasses.__mro__
to check whether it's parent classes are in the formatter dict.register
and unregister
in the first place is that it aligns with the naming in atexit
. But I will update the names.inherit
argument, what is the best default value to set it with? I think both True
and False
make sense, but which one would you prefer?issubclass
function be a better fit? Or does __mro__
contains any other benefits? Edit: one advantage I just noticed is that by using __mro__
, we can preserve the precedence order so that when a class inherits multiple registered types, the formatter inheritance follows the Python behavior.atexit
is a more focused library and has less ambiguity for register
. objprint
on the other hand, could register
multiple things, like "a callback after print" or "an output channel to print". So to avoid the ambiguity, we should be clear about what the user is registering.
Yes both True
and False
make sense, I'm inclined to make it True
by default, I think that's more intuitive - subclasses inheriting the feature of the base class.
Yes, the reason we need to go through the __mro__
is to have a well-defined behavior regardless of the order of the formatter registration. We will always try to find the closest class registered in mro order and that's what Python does.
I have made the requested changes. Here is a brief overview:
inherit
argument is moved to the register_formatter()
, and I have created a custom namedtuple FormatterInfo
to store the formatter and its inherit status. Corresponding condition checkings are also updated.validate_formatter()
has been removed, but the obj_type
checking is kept so that an exception will raise when invalid types are used as arguments.I actually have some other thoughts regarding this feature that I would like to discuss:
Do you think it is useful to allow users to import a dictionary of formatters?
For example, since users can get currently registered formatters by fmt = op.get_formatter()
, they can store it and import it to a different scope using something like op.import_formatter(fmt)
to reuse these registered formatters.
This can be helpful when there needs to be a lot of custom formatters that are being repeatedly used in various files.
Is this a desirable feature to have?
If so, then should I make FormatterInfo
available for import?
@gaogaotiantian
Please take a look when you can and let me know if anything else is needed.
Thank you for your time!
import_formatter()
is not helpful because formatters are already registered globally (process-level). It's not trivial to store the formatters on disk to import later. So, the current status is great.
Related Issue
This PR resolves Issue #77
Description
I have implemented a
register
function to set custom type formatting functions, and aunregister
function to remove it. A decoratorregister_type(obj_type)
is also implemented. Unit tests are included. Readme is updated.