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

feat: add graph subcommand to print dependency graph #264

Merged
merged 3 commits into from
Feb 18, 2024

Conversation

AkashKumar7902
Copy link
Contributor

1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):

  • N
  • Y

solves: #263

2. What is the scope of this PR (e.g. component or file name):

kpm.go
pkg/cmd/cmd_graph.go
pkg/client.go

3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Other

4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):

  • N
  • Y

5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:

  • Unit test
  • Integration test
  • Benchmark (add benchmark stats below)
  • Manual test (add detailed scripts or steps below)
  • Other

@coveralls
Copy link

coveralls commented Feb 10, 2024

Pull Request Test Coverage Report for Build 7946546745

Details

  • -65 of 106 (38.68%) changed or added relevant lines in 2 files are covered.
  • 17 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.3%) to 40.915%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/client/client.go 41 48 85.42%
pkg/cmd/cmd_graph.go 0 58 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/client/client.go 17 56.58%
Totals Coverage Status
Change from base Build 7946161389: -0.3%
Covered Lines: 1664
Relevant Lines: 4067

💛 - Coveralls

@AkashKumar7902 AkashKumar7902 force-pushed the dependency-graph branch 4 times, most recently from 114c5cf to 91bb320 Compare February 11, 2024 07:34
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
@AkashKumar7902
Copy link
Contributor Author

I have implemented my own findSource function as the same is not provided by the library.

@AkashKumar7902 AkashKumar7902 force-pushed the dependency-graph branch 4 times, most recently from 913d5df to fbd007a Compare February 14, 2024 11:51
@AkashKumar7902
Copy link
Contributor Author

AkashKumar7902 commented Feb 14, 2024

@zong-zhe Please take a look

I have implemented two functions here Union and FindSource. The latter is implemented because the function or similar function is not provided by the libraray. The former is an override of the function provided by the library because I have assumed graph may be cyclic and error like "vertex already exist" & "edge already exist" needs to be ignored during cyclic graph construction.

Currently the graph is being built bottom-up in recursion , that's why we are getting the graph as a return value.
We can built the graph in top-down manner but that would require introducing an extra argument to the downloadDeps function. The top-down approach can decrease lines of code and overall complexity.

instead of using downloadDeps for providing two functionality, we can built a clone of downloadDeps function with the name say constructDepGrpah whose only task would be to construct the graph.

Please let me know of any changes.

pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
Signed-off-by: Akash Kumar <meakash7902@gmail.com>
Copy link
Contributor

@Peefy Peefy left a comment

Choose a reason for hiding this comment

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

LGTM

@Peefy Peefy merged commit 0224b99 into kcl-lang:main Feb 18, 2024
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: kpm add command displaying optimization
3 participants