Enhancement: The feature renaming dependency is incomplete #420

Manoramsharma · 2024-07-30T19:39:15Z

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

N
Y

Fixes #397

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

/package/toml.go
/downloader/toml.go
/downloader/toml_test.go
/downloader/test_data/test_data_toml

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

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
[y] Integration test
Benchmark (add benchmark stats below)
Manual test (add detailed scripts or steps below)
Other

Signed-off-by: Manoramsharma <[email protected]>

coveralls · 2024-07-31T10:11:40Z

Pull Request Test Coverage Report for Build 10278727053

Details

6 of 18 (33.33%) changed or added relevant lines in 2 files are covered.
7 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.3%) to 40.541%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/downloader/toml.go	5	17	29.41%

Files with Coverage Reduction	New Missed Lines	%
pkg/client/client.go	3	56.2%
pkg/downloader/toml.go	4	15.98%

Totals
Change from base Build 10278694661:	0.3%
Covered Lines:	3148
Relevant Lines:	7765

💛 - Coveralls

Manoramsharma · 2024-08-01T05:45:16Z

@zong-zhe I have implemented the asked subtask and also changed test according to the implementation need. PTAL !!!

zong-zhe · 2024-08-01T06:05:01Z

pkg/client/client_test.go

-		_ = os.Remove(filepath.Join(pkgPath, "kcl.mod.lock"))
-	}()
-}
+// func TestUpdateWithNoSumCheck(t *testing.T) {


Why remove this test ?

This test is not working and this is not due to my changes.

zong-zhe · 2024-08-01T06:11:22Z

pkg/package/toml.go

@@ -82,7 +82,7 @@ func (dep *Dependencies) MarshalTOML() string {
 const DEP_PATTERN = "%s = %s"

 func (dep *Dependency) MarshalTOML() string {
-	source := dep.Source.MarshalTOML()
+	source := dep.Source.MarshalTOML(dep.Name)


Do not go this way, because the purpose of adding the package field is to support renaming dependency, you need to add a field on the Dependency structure to hold the package name and use this field when serializing to kcl.mod. Instead of simply using dep.Name, the value of package will not be the same as dep.Name when the rename function is implemented later.

zong-zhe · 2024-08-06T03:12:18Z

Hi @Manoramsharma 😃

Is this work still moving forward ？

Manoramsharma · 2024-08-06T04:02:24Z

Hi @Manoramsharma 😃

Is this work still moving forward ？

Hi @zong-zhe ,
Yes I am working on it and will soon come up with a PR.

zong-zhe · 2024-08-08T02:21:48Z

Hi @Manoramsharma 😊

Is this PR finished? You can ping me if it's done or if there's any trouble.

Manoramsharma · 2024-08-08T03:10:28Z

Hi @Manoramsharma 😊

Is this PR finished? You can ping me if it's done or if there's any trouble.

Hi @zong-zhe
I will look into it today and update you accordingly.

Signed-off-by: Manoramsharma <[email protected]>

Manoramsharma · 2024-08-08T06:43:35Z

Hey @zong-zhe, I have added a field named package in the dependency. Now, I am confused because this package is also printed in kcl.lock.mod file. Can you please provide me some guidance on how to add this in packages/toml?

see here,

	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-[dependencies]  [dependencies.flask-demo-kcl-manifests]    name = "flask-demo-kcl-manifests"    full_name = "flask_manifests_0.0.1"    version = "0.0.1"    package = "flask-demo-kcl-manifests"    url = "https://github.com/kcl-lang/flask-demo-kcl-manifests.git"    commit = "ade147b"
        	            	+[dependencies]  [dependencies.flask-demo-kcl-manifests]    name = "flask-demo-kcl-manifests"    full_name = "flask_manifests_0.0.1"    version = "0.0.1"    url = "https://github.com/kcl-lang/flask-demo-kcl-manifests.git"    commit = "ade147b"

zong-zhe · 2024-08-07T13:08:11Z

pkg/downloader/test_data/test_data_toml/expected.toml

@@ -0,0 +1 @@
+{ git = "https://github.com/kcl-lang/flask-demo-kcl-manifests", version = "v0.1.0", package = "flask-demo-kcl-manifests" }


Why changed here ?

zong-zhe · 2024-08-09T02:29:10Z

pkg/package/modfile.go

@@ -193,6 +193,7 @@ type Dependency struct {
 	FullName string `json:"-" toml:"full_name,omitempty"`
 	Version  string `json:"-" toml:"version,omitempty"`
 	Sum      string `json:"-" toml:"sum,omitempty"`
+	Package  string `json:"package" toml:"package,omitempty"`


At present, we do not need to serialize json when the output of this field Package, do not output first, to prevent users from relying on it, causing difficulties for our subsequent changes and updates. So set json:"-"

zong-zhe · 2024-08-09T02:33:25Z

Hey @zong-zhe, I have added a field named package in the dependency. Now, I am confused because this package is also printed in kcl.lock.mod file. Can you please provide me some guidance on how to add this in packages/toml?

see here,

	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-[dependencies]  [dependencies.flask-demo-kcl-manifests]    name = "flask-demo-kcl-manifests"    full_name = "flask_manifests_0.0.1"    version = "0.0.1"    package = "flask-demo-kcl-manifests"    url = "https://github.com/kcl-lang/flask-demo-kcl-manifests.git"    commit = "ade147b"
        	            	+[dependencies]  [dependencies.flask-demo-kcl-manifests]    name = "flask-demo-kcl-manifests"    full_name = "flask_manifests_0.0.1"    version = "0.0.1"    url = "https://github.com/kcl-lang/flask-demo-kcl-manifests.git"    commit = "ade147b"

This part of the situation is normal, and it makes sense to specify the package's original name through the package field in kcl.mod.lock. You can change the test cases to make them pass.

Manoramsharma · 2024-08-09T03:49:44Z

const DEP_PATTERN = "%s = %s"

func (dep *Dependency) MarshalTOML() string {
	source := dep.Source.MarshalTOML()
	var sb strings.Builder
	if len(source) != 0 {
		sb.WriteString(fmt.Sprintf(DEP_PATTERN, dep.Name, source))
	}
	return sb.String()
}

"Hey @zong-zhe, I have one last confusion. How can I add a package in source? Can I change the layout of source so that I can print the package in DEP_PATTERN, or can I somehow manage to pass the package or the entire dependency to MARSHALTOML() of downloader to print the package name there and add it to source?"

zong-zhe · 2024-08-09T04:01:11Z

const DEP_PATTERN = "%s = %s"

func (dep *Dependency) MarshalTOML() string {
	source := dep.Source.MarshalTOML()
	var sb strings.Builder
	if len(source) != 0 {
		sb.WriteString(fmt.Sprintf(DEP_PATTERN, dep.Name, source))
	}
	return sb.String()
}
"Hey @zong-zhe, I have one last confusion. How can I add a package in source? Can I change the layout of source so that I can print the package in DEP_PATTERN, or can I somehow manage to pass the package or the entire dependency to MARSHALTOML() of downloader to print the package name there and add it to source?"

Hi @Manoramsharma 😃

You can adjust the entire rendering toml flow to suit this process. Raises the generation "{}" to top level.

For example :

const DEP_PATTERN = "%s = %s"

func (dep *Dependency) MarshalTOML() string {
	source := dep.Source.MarshalTOML()
	var sb strings.Builder

        # Here is just a example, you need to implement this according to the actual situation
        source := "{"+ source + "package =" + dep.package   "}"

	if len(source) != 0 {
		sb.WriteString(fmt.Sprintf(DEP_PATTERN, dep.Name, source))
	}
	return sb.String()
}

Signed-off-by: Manoramsharma <[email protected]>

Manoramsharma · 2024-08-09T12:26:17Z

@zong-zhe Please review this one i have done the suggested changes.

pkg/package/toml.go

Signed-off-by: Manoramsharma <[email protected]>

Manoramsharma · 2024-08-13T11:32:44Z

@zong-zhe I have implemented the required changes. Previously, when encountering the opt.NewPkgName option, we only updated the dependency name (d.Name = newPkgName) to the new package name. Now, in addition to setting the dependency name, I have also configured the package field to use the old dependency name (package=d.Name).

Peefy · 2024-08-20T07:33:05Z

cc @zong-zhe Could you help review it again?

Manoramsharma and others added 4 commits July 31, 2024 00:56

add a package field to kcl.mod

936f1f1

Signed-off-by: Manoramsharma <[email protected]>

updated package toml

a326ac0

Signed-off-by: Manoramsharma <[email protected]>

Fixes some tests

a8b2647

Signed-off-by: Manoramsharma <[email protected]>

changed some of the test

d66e761

Signed-off-by: Manoramsharma <[email protected]>

zong-zhe reviewed Aug 1, 2024

View reviewed changes

Manoramsharma added 4 commits August 6, 2024 13:10

Merge branch 'main' into patch-rename-branch

b52baed

Merge branch 'main' into patch-rename-branch

d7efcee

Merge branch 'main' into patch-rename-branch

e3cc662

Merge branch 'main' into patch-rename-branch

5fc77c3

[feat]: added support for package name

78b09ed

Signed-off-by: Manoramsharma <[email protected]>

zong-zhe reviewed Aug 9, 2024

View reviewed changes

added package in kcl.mod

9b81593

Signed-off-by: Manoramsharma <[email protected]>

Manoramsharma requested a review from zong-zhe August 9, 2024 12:26

zong-zhe reviewed Aug 12, 2024

View reviewed changes

pkg/package/toml.go Show resolved Hide resolved

Manoramsharma added 2 commits August 12, 2024 12:11

Merge branch 'main' into patch-rename-branch

142117b

refactoreds ource to contain package name

ad6f168

Signed-off-by: Manoramsharma <[email protected]>

Manoramsharma requested a review from zong-zhe August 12, 2024 08:41

Manoramsharma added 2 commits August 13, 2024 16:37

Merge branch 'main' into patch-rename-branch

4906ca1

Added the package name only with rename flag

5095615

Signed-off-by: Manoramsharma <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: The feature renaming dependency is incomplete #420

Enhancement: The feature renaming dependency is incomplete #420

Manoramsharma commented Jul 30, 2024

coveralls commented Jul 31, 2024 •

edited

Loading

Manoramsharma commented Aug 1, 2024

zong-zhe Aug 1, 2024

Manoramsharma Aug 1, 2024

zong-zhe Aug 1, 2024

zong-zhe commented Aug 6, 2024

Manoramsharma commented Aug 6, 2024

zong-zhe commented Aug 8, 2024

Manoramsharma commented Aug 8, 2024

Manoramsharma commented Aug 8, 2024 •

edited

Loading

zong-zhe Aug 7, 2024

zong-zhe Aug 9, 2024

zong-zhe commented Aug 9, 2024

Manoramsharma commented Aug 9, 2024 •

edited

Loading

zong-zhe commented Aug 9, 2024

Manoramsharma commented Aug 9, 2024

Manoramsharma commented Aug 13, 2024

Peefy commented Aug 20, 2024

		@@ -0,0 +1 @@
		{ git = "https://github.com/kcl-lang/flask-demo-kcl-manifests", version = "v0.1.0", package = "flask-demo-kcl-manifests" }

Enhancement: The feature renaming dependency is incomplete #420

Are you sure you want to change the base?

Enhancement: The feature renaming dependency is incomplete #420

Conversation

Manoramsharma commented Jul 30, 2024

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):

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

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

coveralls commented Jul 31, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10278727053

Details

💛 - Coveralls

Manoramsharma commented Aug 1, 2024

zong-zhe Aug 1, 2024

Choose a reason for hiding this comment

Manoramsharma Aug 1, 2024

Choose a reason for hiding this comment

zong-zhe Aug 1, 2024

Choose a reason for hiding this comment

zong-zhe commented Aug 6, 2024

Manoramsharma commented Aug 6, 2024

zong-zhe commented Aug 8, 2024

Manoramsharma commented Aug 8, 2024

Manoramsharma commented Aug 8, 2024 • edited Loading

zong-zhe Aug 7, 2024

Choose a reason for hiding this comment

zong-zhe Aug 9, 2024

Choose a reason for hiding this comment

zong-zhe commented Aug 9, 2024

Manoramsharma commented Aug 9, 2024 • edited Loading

zong-zhe commented Aug 9, 2024

Manoramsharma commented Aug 9, 2024

Manoramsharma commented Aug 13, 2024

Peefy commented Aug 20, 2024

coveralls commented Jul 31, 2024 •

edited

Loading

Manoramsharma commented Aug 8, 2024 •

edited

Loading

Manoramsharma commented Aug 9, 2024 •

edited

Loading