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

improve json output for CommandOutput::Table #1723

Merged

Conversation

biryukovmaxim
Copy link
Contributor

{"headers": [...], "values": [[...]]} -> [{"header": "value"}]
resolve #906

@biryukovmaxim biryukovmaxim requested a review from a team November 28, 2021 21:36
filipgolem
filipgolem previously approved these changes Nov 29, 2021
Copy link
Contributor

@filipgolem filipgolem left a comment

Choose a reason for hiding this comment

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

Good job. I would just rename some variables (see suggestions).

filipgolem
filipgolem previously approved these changes Nov 30, 2021
Copy link
Contributor

@filipgolem filipgolem left a comment

Choose a reason for hiding this comment

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

Approved. Consider changing "not any not equal" to "all equal": #1723 (comment)

{"headers": [...], "values": [[...]]} -> [{"header": "value"}]
resolve golemfactory#906
@maaktweluit
Copy link
Contributor

Hey @biryukovmaxim

Thanks for your contribution!
The PR is approved but we need 3 more steps before we can merge this:

  • Updating to master
  • Checking if the automated test goth works properly after merging
  • Check if there is any other tooling dependent on this

The first 2 should be simple, after updating to master we can see the goth build run and make an issue in the goth repo when it fails.

For the last one i made a note on our board, we will look into this and update you here :)

@etam
Copy link
Contributor

etam commented Mar 21, 2022

I would suggest a further refactoring (may be as a next step after this PR), to change ResponseTable in core/serv-api/src/lib.rs from

pub struct ResponseTable {
    pub columns: Vec<String>,
    pub values: Vec<serde_json::Value>,
}

to

pub struct ResponseTable<T: serde::Serialize>(Vec<T>);

or something like that.

@etam etam enabled auto-merge March 29, 2022 12:54
@etam etam mentioned this pull request Mar 29, 2022
@nieznanysprawiciel
Copy link
Contributor

We updated all tools depending on this code.
PR will be merged after tests will pass

@etam etam merged commit eb31aad into golemfactory:master Mar 30, 2022
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.

better json output from commands
6 participants