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

Move consts, ipfs, and ipld to pkg #508

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Move consts, ipfs, and ipld to pkg #508

merged 4 commits into from
Aug 26, 2021

Conversation

evan-forbes
Copy link
Member

Description

This super simple PR moves the consts, ipld, and ipfs packages to pkg and that's it. I was strict in limiting changes of this PR in hopes of making the review process for this and future PRs simpler.

Part of #506

@evan-forbes evan-forbes self-assigned this Aug 23, 2021
@evan-forbes evan-forbes mentioned this pull request Aug 23, 2021
4 tasks
Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

I think it's OK to hold off moving these into a separate repo until we actually run into the problem of circular dependencies. The pkg dir always stirred some controversy around go devs (e.g. see: golang-standards/project-layout#10). I do think it's useful to signal: "hey these modules are specifically meant to be used as a library".

What is the next step with this? My understanding is that a lot of the code that currently uses this will actually be not using it anymore; is that right?

Would also like to hear @Wondertan's opinion on this.

@evan-forbes
Copy link
Member Author

What is the next step with this? My understanding is that a lot of the code that currently uses this will actually be not using it anymore; is that right?

yeah, we'd definitely remove and refactor some code, this is only an intermediate step that will hopefully make the review process for the next steps easier. Those next steps would be to complete #506 and #505. Eventually, I think we'd move things into their own package as discussed in #296

Copy link
Member

@liamsi liamsi left a comment

Choose a reason for hiding this comment

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

This looks good to me but I want @Wondertan to review it before merging as it might collide with some of his plans in celestiaorg/celestia-node#21 ?

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

Let's move it!

Also, this doesn't go against my plans. The next step for the da/ipld would be not to depend on Tendermint types, as done in #427, and moving is not an issue.

@evan-forbes evan-forbes merged commit c4f2a98 into master Aug 26, 2021
@evan-forbes evan-forbes deleted the evan/start-pkging branch August 26, 2021 13:59
evan-forbes added a commit that referenced this pull request Sep 16, 2021
* move ipfs and ipld packages to new pkg/da/

* moved consts to pkg

* clean up
evan-forbes added a commit that referenced this pull request Sep 17, 2021
* Move consts, ipfs, and ipld to pkg (#508)

* move ipfs and ipld packages to new pkg/da/

* moved consts to pkg

* clean up

* go mod tidy
evan-forbes added a commit that referenced this pull request Sep 20, 2021
* Move consts, ipfs, and ipld to pkg (#508)

* move ipfs and ipld packages to new pkg/da/

* moved consts to pkg

* clean up

* go mod tidy
evan-forbes added a commit that referenced this pull request Sep 24, 2021
* Move consts, ipfs, and ipld to pkg (#508)

* move ipfs and ipld packages to new pkg/da/

* moved consts to pkg

* clean up

* go mod tidy
evan-forbes added a commit that referenced this pull request Apr 26, 2022
* Move consts, ipfs, and ipld to pkg (#508)

* move ipfs and ipld packages to new pkg/da/

* moved consts to pkg

* clean up

* go mod tidy
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