Skip to content

Da/metadata#72

Merged
tadad merged 7 commits intodevelopfrom
da/metadata
Feb 14, 2024
Merged

Da/metadata#72
tadad merged 7 commits intodevelopfrom
da/metadata

Conversation

@tadad
Copy link
Copy Markdown
Contributor

@tadad tadad commented Feb 13, 2024

No description provided.

@tadad tadad requested a review from nick1udwig February 13, 2024 21:27
Copy link
Copy Markdown
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

A few comments:

  1. Let's point this at develop branch.
  2. I'd prefer to revert db604ea
    There are a number of places in the code where rustfmt makes it less readable imo, e.g.
    https://github.com/kinode-dao/kit/blob/db604eaf5241acea442d29fa3d38830fae8b9617/src/build/mod.rs#L112-L116
    where
    .stdout(if verbose { Stdio::inherit() } else { Stdio::null() })
    
    is changed to
    .stdout(if verbose {
        Stdio::inherit()
    } else {
        Stdio::null()
    })
    
  3. Let's lock in a rev for kinode_process_lib: it can point to a branch. I like to be able to test PRs.

Comment thread src/start_package/mod.rs Outdated
@@ -1,3 +1,4 @@
use kinode_process_lib::kernel_types::Erc721Metadata;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would prefer this be in a third grouping of imports after the one starting with serde_json. The general convention I like to follow is:

  1. Alphabetized within a group.
  2. First group: "stdlibs"
  3. Second group: imported libs/crates
  4. Third group: our libs/crates

and so on: from more "general"/widely distributed to more "specific"/local.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, one more down in its own grp, e.g.

use std::foo;
...

use serde_json::bar;
...

use kinode_process_lib::baz;
...

use super::inject_message;
...

@tadad tadad changed the base branch from master to develop February 13, 2024 21:48
Copy link
Copy Markdown
Member

@nick1udwig nick1udwig left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@tadad tadad merged commit cc737aa into develop Feb 14, 2024
@nick1udwig nick1udwig deleted the da/metadata branch February 14, 2024 20:27
@nick1udwig
Copy link
Copy Markdown
Member

@tadad I realize I should have asked for a docs PR associated with this one before merge. I've opened hyperware-ai/hyperware-book#132 to track it.

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