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

[WIP] kpm fixes the order of dependencies and format in kcl.mod and kcl.mod.lock #336

Closed
wants to merge 2 commits into from

Conversation

officialasishkumar
Copy link
Contributor

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

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

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

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9228658894

Details

  • 0 of 13 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 43.043%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/package/package.go 0 1 0.0%
pkg/package/modfile.go 0 12 0.0%
Totals Coverage Status
Change from base Build 9221341511: -0.1%
Covered Lines: 2447
Relevant Lines: 5685

💛 - Coveralls

@@ -146,6 +147,21 @@ type Dependencies struct {
Deps map[string]Dependency `json:"packages" toml:"dependencies,omitempty"`
}

// SortMapByKey will sort the dependencies by key.
func (deps *Dependencies) SortMapByKey() {
Copy link
Contributor

@Peefy Peefy May 25, 2024

Choose a reason for hiding this comment

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

I think we should use an ordered dictionary for serialization/deserialization instead of simply sorting, as KCL users often do not want unexpected changes to occur.

Besides, [dependencies] section and [profile] section lost the expect newline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Peefy , i've done some changes at #334, please have a look over them if they are in the right direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Peefy , i've done some changes at #334, please have a look over them if they are in the right direction.

@d4v1d03 OK. But we should avoid commenting on irrelevant PRs.

@Peefy
Copy link
Contributor

Peefy commented May 28, 2024

Add some comments. cc @zong-zhe Could you help review it?

@officialasishkumar
Copy link
Contributor Author

I will do the required changes.

@@ -175,6 +175,7 @@ func (kclPkg *KclPkg) LocalVendorPath() string {
// updateModAndLockFile will update kcl.mod and kcl.mod.lock
func (kclPkg *KclPkg) UpdateModAndLockFile() error {
// Generate file kcl.mod.
kclPkg.ModFile.Dependencies.SortMapByKey()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @officialasishkumar 😃

Good job ! But I have a few different ideas about this feature.

The go community maybe have libraries to help us solve this problem. Try to research some of the libraries and see the possible problems with using them. You can update the results of the research in the issue.

I found a third-party library, https://github.com/elliotchance/orderedmap, but when the library is serialized into a kcl.mod file, there may be some problems, this need you to continue to research.

It's not that your method is wrong, it's just that we need some evidence to show that this method is more appropriate than other methods.

@officialasishkumar officialasishkumar changed the title kpm fixes the order of dependencies and format in kcl.mod and kcl.mod.lock [WIP] kpm fixes the order of dependencies and format in kcl.mod and kcl.mod.lock Jun 3, 2024
@Peefy
Copy link
Contributor

Peefy commented Jun 3, 2024

PR conflict cc @officialasishkumar

@officialasishkumar officialasishkumar marked this pull request as draft June 3, 2024 07:32
@Peefy
Copy link
Contributor

Peefy commented Jun 3, 2024

I noticed the function still use the unordered map.

func (local *Local) UnmarshalModTOML(data interface{}) error {
	meta, ok := data.(map[string]interface{})
	if !ok {
		return fmt.Errorf("expected map[string]interface{}, got %T", data)
	}
	if v, ok := meta[LOCAL_PATH_FLAG].(string); ok {
		local.Path = v
	}
	return nil
}

@officialasishkumar
Copy link
Contributor Author

@Peefy The migration is not yet completed.

Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
@zong-zhe
Copy link
Contributor

zong-zhe commented Jun 18, 2024

Hi @officialasishkumar 😄

This PR conflicts with the main branch, you may need to squash some commits and rebase the main branch.

@zong-zhe
Copy link
Contributor

Hi @officialasishkumar 😄

This PR conflicts with the main branch. Are you still working on it?

@officialasishkumar
Copy link
Contributor Author

Hi @officialasishkumar 😄

This PR conflicts with the main branch. Are you still working on it?

Yeah, I will complete it. Sorry my exams were running for a few weeks. Now it's over.

@zong-zhe
Copy link
Contributor

zong-zhe commented Jul 4, 2024

Hi @officialasishkumar 😄

I finished this issue for the 0.9.0 release, #373
You'll be able to focus on the LFX now.

@zong-zhe zong-zhe closed this Jul 4, 2024
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.

Enhancement: kpm add command displaying optimization
5 participants