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

manifest: support path dependencies #1048

Merged
merged 6 commits into from
Dec 24, 2024

Conversation

wx257osn2
Copy link
Contributor

@wx257osn2 wx257osn2 commented Dec 21, 2024

This PR introduces path dependency like Cargo's one.
Path dependency specify a local directory as dependency.
Unlike Cargo's one,

  • currently poac doesn't care about poac.toml in the path.
    • Like git dependency, path dependency implemented by this PR supports only header-only library.
  • poac doesn't support version for published package.

This PR doesn't contain poac add --path sub-command option.

Poac with this PR can allow poac.toml like below:

[dependencies]
local_header_only_library = {path = "./path/to/library"}

@wx257osn2 wx257osn2 force-pushed the implement-local-dependency branch from cec12cf to c8874a0 Compare December 21, 2024 08:34
Copy link
Member

@ken-matsui ken-matsui left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. This looks nice code-wise, but let’s address 2 things:

  1. Use path instead of local (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-path-dependencies)
  2. Follow CONTRIBUTING.md, especially Documentation, Commit Message, and Pull Request Style.

@wx257osn2 wx257osn2 changed the title Add LocalDependency Support path depndency Dec 22, 2024
@wx257osn2
Copy link
Contributor Author

  1. Use path instead of local (https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-path-dependencies)

done.

  1. Follow CONTRIBUTING.md, especially Documentation, Commit Message, and Pull Request Style.

What exactly will I need to do?

  • The suggestion from cpplint (missing include) has been fixed.
  • To describe the document, I need to implement poac add --path , however this PR doesn't handle it currently.
  • What would you like to see added to the commit message? Do you want it more document-like? If you had felt difficulty to review due to my specific commit message, I'd like to know.
    • In my opinion, it is reasonable to improve description of PR, but not about each commit message, because poac project merges PR using squash-merge, so no matter how each commit message is written, they won't remain in main .
  • I thought that there is no problem with CI or commit structure. Is there any problem about Pull Request Style? Do you mean the description should be more increased?

@wx257osn2 wx257osn2 force-pushed the implement-local-dependency branch from 59f6275 to 8400fcc Compare December 22, 2024 15:14
@ken-matsui
Copy link
Member

You can update the documentation without mentioning poac add just like the README before adding poac add.

It's not mandatory to update each commit message, but improving future messages saves me tons of time keeping track of progression. Can you follow this for the PR title and description? This documentation is meant to be reviewed before creating a PR, as the PR title and description are generated from the commit message.

@wx257osn2
Copy link
Contributor Author

You can update the documentation without mentioning poac add just like the README before adding poac add.

You mean to bring back the description to edit poac.toml after poac add (and description about poac add must be left as is), right? I will try it.

Can you follow this for the PR title and description?

I've thought what the description doesn't contain, and I've realized the description doesn't explain what is path dependency and how it behave. I'll add some description about it.

@ken-matsui
Copy link
Member

You can update the documentation without mentioning poac add just like the README before adding poac add.

You mean to bring back the description to edit poac.toml after poac add (and description about poac add must be left as is), right? I will try it.

Yes, whichever after or before, depending on what makes more sense.

Can you follow this for the PR title and description?

I've thought what the description doesn't contain, and I've realized the description doesn't explain what is path dependency and how it behave. I'll add some description about it.

The key point for the PR title is component: what you did. You can follow https://git-scm.com/docs/SubmittingPatches#describe-changes for the PR title for more details and the PR description.

@wx257osn2 wx257osn2 changed the title Support path depndency Manifest: Support path depndency Dec 23, 2024
@ken-matsui ken-matsui changed the title Manifest: Support path depndency manifest: support path dependencies Dec 24, 2024
Copy link
Member

@ken-matsui ken-matsui left a comment

Choose a reason for hiding this comment

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

Thanks!

@ken-matsui ken-matsui merged commit a46d104 into cabinpkg:main Dec 24, 2024
18 checks passed
@wx257osn2 wx257osn2 deleted the implement-local-dependency branch December 24, 2024 09:19
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