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

feat(framework) Add JSON formatting to flwr ls output #4610

Open
wants to merge 14 commits into
base: introduce-json-format
Choose a base branch
from

Conversation

chongshenng
Copy link
Contributor

@chongshenng chongshenng commented Dec 2, 2024

Merge afte #4613

@chongshenng chongshenng changed the base branch from main to introduce-json-format December 2, 2024 12:38
src/py/flwr/common/logger.py Show resolved Hide resolved
src/py/flwr/cli/ls.py Show resolved Hide resolved
src/py/flwr/cli/ls.py Show resolved Hide resolved
src/py/flwr/cli/ls.py Show resolved Hide resolved
src/py/flwr/cli/ls.py Outdated Show resolved Hide resolved
if suppress_output:
restore_output()
e_message = captured_output.getvalue()
_print_json_error(e_message, e)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does that mean no exception message will be available in default format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if there're any exceptions in default mode, the errors are not redirected.

Copy link
Contributor

Choose a reason for hiding this comment

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

But all exceptions are captured, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the error messages are not redirected. So the if statement doesn't affect the default mode.

Copy link
Contributor

@panh99 panh99 Dec 3, 2024

Choose a reason for hiding this comment

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

I don’t think so. All exceptions are captured here, but if you look at the code above, it only captures ValueError. This approach will suppress all other exceptions. For example, if there’s a gRPC exception when using the stub object, users will see the exception in --format json mode, but nothing will appear in the default mode. See below:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_print_json_error(e_message, e)
_print_json_error(e_message, e)
else:
raise

Console().print_json(
json.dumps(
{
"success": "false",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"success": "false",
"success": False,

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it be a true bool?

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.

3 participants