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

Add subcmd add #914

Merged
merged 2 commits into from
Feb 25, 2024
Merged

Add subcmd add #914

merged 2 commits into from
Feb 25, 2024

Conversation

SunPodder
Copy link
Contributor

No description provided.

@SunPodder SunPodder marked this pull request as draft February 24, 2024 02:02
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 for your time to work on this! I put several comments for performance and style consistency.

src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.hpp Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.hpp Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
@SunPodder SunPodder force-pushed the add_cmd branch 2 times, most recently from 213f43c to 2d3f608 Compare February 24, 2024 11:54
@SunPodder SunPodder marked this pull request as ready for review February 24, 2024 11:54
@SunPodder SunPodder requested a review from ken-matsui February 24, 2024 11:55
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 your time again! Just a few things. Can you please also deal with the clang-tidy error?

src/Cli.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
@SunPodder
Copy link
Contributor Author

All fixed including the clang-tidy error

@ken-matsui
Copy link
Member

ken-matsui commented Feb 24, 2024

@SunPodder Sorry, there are more clang-tidy errors. Could you please address them? (After that, LGTM)

@SunPodder SunPodder force-pushed the add_cmd branch 2 times, most recently from f6573ac to a06df02 Compare February 25, 2024 15:11
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
src/Cmd/Add.cc Outdated Show resolved Hide resolved
Allows users to add project dependencies to poac.toml from the commandline
@SunPodder
Copy link
Contributor Author

image

Sorry, it must be annoying for you reviewing the same mistakes. clang-tidy throws errors on my machine even for existing codes which are correct and passes on ci. So it's hard to make sure everything is fine :)

@ken-matsui
Copy link
Member

No worries! Which version of clang-tidy are you using?

@SunPodder
Copy link
Contributor Author

No worries! Which version of clang-tidy are you using?

clang-tidy-15

@ken-matsui
Copy link
Member

It seems that clang-tidy-17 fixed the problem, just fyi: llvm/llvm-project#46097.

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 your contribution!

@ken-matsui ken-matsui merged commit 3c352da into cabinpkg:main Feb 25, 2024
14 checks passed
@ken-matsui
Copy link
Member

@SunPodder Can you also update README.md for this command?

@SunPodder
Copy link
Contributor Author

Sure

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