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

adding structured output option #10

Merged
merged 3 commits into from
Jun 13, 2023
Merged

adding structured output option #10

merged 3 commits into from
Jun 13, 2023

Conversation

tomreitz
Copy link
Collaborator

@tomreitz tomreitz commented Jun 8, 2023

This PR implements the structured output portion of this issue. (A separate one will be coming to implement the exit code portion.) It adds the --results-file flag which allows you to specify an output to file to which earthmover will write a structured JSON run summary.

Example JSON output can be found in the addition to the README in this PR.

…g; see description and sample output added to README
@tomreitz tomreitz requested a review from jayckaiser June 8, 2023 22:24
lightbeam/send.py Outdated Show resolved Hide resolved
lightbeam/send.py Outdated Show resolved Hide resolved
@@ -109,10 +156,27 @@ async def do_post(self, endpoint, file_name, data, client, line, hash):
# warn about errors
if response.status not in [ 200, 201 ]:
message = str(response.status) + ": " + util.linearize(body)

# update run metadata...
if "failed_statuses" not in self.metadata["resources"][endpoint].keys():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider isolating the failed statuses dict as a variable to streamline updates.

failed_statuses_dict = self.metadata["resources"][endpoint].get("failed_statuses", {})

if response.status not in failed_statuses_dict:
    failed_statuses_dict.update({response.status: {}})

if message not in failed_statuses_dict[response.status]:
    failed_statuses_dict[response.status].update({message: {}})

if "files" not in failed_statuses_dict[response.status][message]:
    failed_status_dict[response.status][message].update({"files": {}})

if file_name not in failed_statuses_dict[response.status][message]["files"]:
    failed_statuses_dict[response.status][message]["files"].update({file_name: {}})

if "line_numbers" not in failed_statuses_dict[response.status][message]["files"][file_name]:
    failed_statuses_dict[response.status][message]["files"][file_name].update({"line_numbers": []})

failed_statuses_dict[response.status][message]["files"][file_name]["line_numbers"].append(line)

Additionally, this is a very example of a defaultdict use-case, but it's multiple layers nested which might be difficult to set-up. If we were able to do so, it would convert this entire code-block to the following:

self.metadata["resources"][endpoint]["failed_statuses"][response.status][message]["files"][file_name]["line_numbers"].append(line)

I'm going to play around with this concept a little more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you're saying, and this could be cleaner. One little bug in what you posted above, I think I still need to check for values in objects' .keys(), since they're nested dicts. You have

if response.status not in failed_statuses_dict:
    failed_statuses_dict.update({response.status: {}})

which I think still needs to be

if response.status not in failed_statuses_dict.keys():
    failed_statuses_dict.update({response.status: {}})

(etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jayckaiser I made another update along these lines in this commit and I tested it. If this is good enough for now, I can merge and we could consider refining later?

@tomreitz tomreitz merged commit 15de2a0 into main Jun 13, 2023
@tomreitz tomreitz deleted the feature/structured_output branch June 13, 2023 20:45
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.

2 participants