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.

Sign up for GitHub

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

Add custom formatter for Fire result #345

Merged
merged 6 commits into from
Apr 16, 2022
Merged

Add custom formatter for Fire result #345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ceda820
add formatter argument
beasteers Jul 7, 2021
ffc8077
added another test case
beasteers Jul 7, 2021
8a4963d
improved test case
beasteers Jul 7, 2021
e837db2
rename formatter= to serialize=
beasteers Dec 5, 2021
2783342
raise error if serialize is not callable
beasteers Dec 6, 2021
1b29766
improve serialize non-callable error message and add test case
beasteers Dec 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions fire/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def main(argv):
import asyncio # pylint: disable=import-error,g-import-not-at-top # pytype: disable=import-error


def Fire(component=None, command=None, name=None):
def Fire(component=None, command=None, name=None, serialize=None):
"""This function, Fire, is the main entrypoint for Python Fire.

Executes a command either from the `command` argument or from sys.argv by
Expand Down Expand Up @@ -164,7 +164,7 @@ def Fire(component=None, command=None, name=None):
raise FireExit(0, component_trace)

# The command succeeded normally; print the result.
_PrintResult(component_trace, verbose=component_trace.verbose)
_PrintResult(component_trace, verbose=component_trace.verbose, serialize=serialize)
result = component_trace.GetResult()
return result

Expand Down Expand Up @@ -241,12 +241,19 @@ def _IsHelpShortcut(component_trace, remaining_args):
return show_help


def _PrintResult(component_trace, verbose=False):
def _PrintResult(component_trace, verbose=False, serialize=None):
"""Prints the result of the Fire call to stdout in a human readable way."""
# TODO(dbieber): Design human readable deserializable serialization method
# and move serialization to its own module.
result = component_trace.GetResult()

# Allow users to modify the return value of the component and provide
# custom formatting.
if serialize:
if not callable(serialize):
raise FireError("serialize argument {} must be empty or callable.".format(serialize))
result = serialize(result)
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to handle the not-callable case too.
I think if we cannot call serialize, we raise a FireError.

Copy link
Contributor Author

@beasteers beasteers Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we just do:

if serialize:
    result = serialize(result)

which will just raise whatever error the object would raise if tried to be called (probably typeerror)

And this would allow any falsey value to disable serialize (None, False, etc)

Copy link
Contributor Author

@beasteers beasteers Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

addressed - it will now raise FireError("serialize argument {} must be empty or callable.".format(serialize))

beasteers marked this conversation as resolved.
Show resolved Hide resolved

if value_types.HasCustomStr(result):
# If the object has a custom __str__ method, rather than one inherited from
# object, then we use that to serialize the object.
Expand Down
24 changes: 24 additions & 0 deletions fire/core_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,30 @@ def testClassMethod(self):
7,
)

def testCustomSerialize(self):
def serialize(x):
if isinstance(x, list):
return ', '.join(str(xi) for xi in x)
if isinstance(x, dict):
return ', '.join('{}={!r}'.format(k, v) for k, v in x.items())
if x == 'special':
return ['SURPRISE!!', "I'm a list!"]
Copy link
Member

Choose a reason for hiding this comment

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

The name "serialize" implies that the output of serialize is either bytes or a string.
But that's not what we're requiring.
I wonder if there's a better name (maybe that's "format" after all.)
I'll think on this, but curious for your thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "process" or "finalize"...
I'll keep thinking.

Copy link
Member

Choose a reason for hiding this comment

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

Forgive my unpolished thinking aloud:

Maybe "serialize" is fine, and the logic is: "if you don't finish serializing it, we'll continue serializing it for you using the default serializer."
And then "display" will only ever be given bytes or a string, rather than being directly given the output of serialize.
One challenge with this is that it disallows different serialization for different display modes.
We could (in a subsequent PR) allow serialize to accept the "kind" parameter too to reenable this capability.

Copy link
Contributor Author

@beasteers beasteers Dec 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm I mean I personally like format too, but serialize can also work and I think that rationale is fine

One challenge with this is that it disallows different serialization for different display modes

I'm not sure I understand the motivation behind this. Do you mean that you'd want the display mode to have its own default serializer to fall back on? If that's the case, then maybe there could be another mode of serializer override tied to the display object.

# could also be defined as a class if you want
def mydisplay(x):
    print(x)
def serialize(x):
    return x
mydisplay.serialize = serialize

# then internally _Fire(..., display=mydisplay)
default_serialize = getattr(display, 'serialize', None)
if default_serialize:
    result = default_serialize(result)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

side note: one thing that would be nice is if we could have some sort of override via the CLI where the user could do --format json or --format csv or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some notes about the naming - I like serialize and format because they both imply a string result. I think there might be a problem with process or finalize cuz they could imply that you have the ability to modify the result after each iteration (after each getattr / getitem / call).

I can't think of any other words better than those two tho.

Maybe "serialize" is fine, and the logic is: "if you don't finish serializing it, we'll continue serializing it for you using the default serializer."

I think this is fine because even if the user doesn't realize that they can't return a string from it, it's not going to break anything, it might just make a little more work for them.

return x

ident = lambda x: x

with self.assertOutputMatches(stdout='a, b', stderr=None):
result = core.Fire(ident, command=['[a,b]'], serialize=serialize)
with self.assertOutputMatches(stdout='a=5, b=6', stderr=None):
result = core.Fire(ident, command=['{a:5,b:6}'], serialize=serialize)
with self.assertOutputMatches(stdout='asdf', stderr=None):
result = core.Fire(ident, command=['asdf'], serialize=serialize)
with self.assertOutputMatches(stdout="SURPRISE!!\nI'm a list!\n", stderr=None):
result = core.Fire(ident, command=['special'], serialize=serialize)
with self.assertRaises(core.FireError):
core.Fire(ident, command=['asdf'], serialize=55)


@testutils.skipIf(six.PY2, 'lru_cache is Python 3 only.')
def testLruCacheDecoratorBoundArg(self):
self.assertEqual(
Expand Down