update to avoid hard coding of the artifactType #30

IsabellaDev · 2020-10-17T02:40:31Z

Hi, sorry for the delay, please check if this what you need, let me know if you need me to do any modification. Thanks.

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

ashokponkumar · 2020-10-17T05:17:32Z

main.go

-	helmTargetArtifactTypeValue := "Helm"
-	yamlsTargetArtifactTypeValue := "Yamls"
-	knativeTargetArtifactTypeValue := "Knative"
-
 	re := regexp.MustCompile(`artifacttype:\\s.*`)


Can we possibly account for scenarios where mistakenly more than one space is left by using say \s*

Also can we search for [^\s]+ as the string to search for?

Also please chande artifacttype to artifactType. It was a bug from before.

Hey, I made some changes, please check if it satisfied the need. I removed one '' from your regexp pattern, it seems only '\s' should meet the requirement to represent the whitespace from my test. Let me know if you need any updates. Thanks.

ashokponkumar · 2020-10-17T05:19:21Z

main.go

-	} else if strings.HasSuffix(artifactTypematch, helmTargetArtifactTypeValue) {
-		artifactType = helmTargetArtifactTypeValue
-	}
+	match := []rune(artifactTypematch)


Before this we could possibly check if the match was successful or empty. If empty we can use s hard-coded empty vslue like "".

Sure, you are right, there should be a check to see if the match is successful or not. So if it's empty, you want the artifactType to be set to "", is that correct?

ashokponkumar · 2020-10-17T05:20:44Z

main.go

-		artifactType = helmTargetArtifactTypeValue
-	}
+	match := []rune(artifactTypematch)
+	artifactType := string(match[16:len(match)])


Instead of hardcoding 16, can we ectract out the non spaced string from suffiix? By say using a regex? What do you think?

Hi, 16 is the length of the former part of the string, it has nothing to do with the suffix. So if you want me to remove all possible whitespace, I can try to make the change. Or maybe you prefer to use regex groups to avoid such hardcoding like 16?

Yes, Using regex groups would be a easier approach.

ashokponkumar · 2020-10-17T05:21:44Z

Hi, sorry for the delay, please check if this what you need, let me know if you need me to do any modification. Thanks.

Thanks for the fix. Have suggested msome changes, please have a look.

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

ashokponkumar · 2020-10-18T06:10:01Z

main.go


-	helmTargetArtifactTypeValue := "Helm"
-	yamlsTargetArtifactTypeValue := "Yamls"
-	knativeTargetArtifactTypeValue := "Knative"
-
-	re := regexp.MustCompile(`artifacttype:\\s.*`)
+	re := regexp.MustCompile(`artifactType:\s.*`)
 	artifactTypematch := re.FindString(plan)
-	artifactType := yamlsTargetArtifactTypeValue
-	if strings.HasSuffix(artifactTypematch, knativeTargetArtifactTypeValue) {
-		artifactType = knativeTargetArtifactTypeValue
-	} else if strings.HasSuffix(artifactTypematch, helmTargetArtifactTypeValue) {
-		artifactType = helmTargetArtifactTypeValue
+	var artifactType string
+	if artifactTypematch == "" {
+		artifactType = ""
+	} else {
+		match := []rune(artifactTypematch)
+		name := string(match[len("artifactType:"):len(match)])
+		artifactType = strings.Replace(name, " ", "", -1)
 	}
+


How about something like

var artifactType re := regexp.MustCompile(`artifactType:\s*([^\s]*)`) matches = rgx.FindStringSubmatch(plan) if len(matches) > 0 { artifactType = matches[0] }

Hmm,that's a smart solution. But is [^\s]* necessary, since \s* should be able to catch all white spaces behind : I think?

It will help ignore the space at the end. Also now that I think about it a + is better than a * there

I made some changes, please take a look. [^\s]* would cause no match if there is any whitespace in the latter part. So I used (.+) instead, and then removed any possible whitespace in it.

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

ashokponkumar · 2020-10-19T07:23:40Z

main.go

-		artifactType = knativeTargetArtifactTypeValue
-	} else if strings.HasSuffix(artifactTypematch, helmTargetArtifactTypeValue) {
-		artifactType = helmTargetArtifactTypeValue
+	re := regexp.MustCompile(`artifactType:\s*(.+)`)


Suggested change

re := regexp.MustCompile(`artifactType:\s*(.+)`)

re := regexp.MustCompile(`artifactType:\s*([^\s]+)`)

Won't this end up choosing a contiguous set of non space characters like what we want? The issue with the previous regex I had suggested was the user of * which was incorrect.

If there would be no whitespace in the words between, [^\s]+ can give the correct answer. But if the string is like:
"artifactType: he l m", it will only take "he", not "helm". If "he" is what you expected, I can change it to '[^\s]+'.
Please let me know, thanks.

ashokponkumar · 2020-10-19T07:29:26Z

main.go

+	if len(artifactTypematch) == 0 {
+		artifactType = ""
+	} else {
+
+		artifactType = strings.ReplaceAll(artifactTypematch[1], " ", "")
 	}
+


Suggested change

if len(artifactTypematch) == 0 {

artifactType = ""

} else {

artifactType = strings.ReplaceAll(artifactTypematch[1], " ", "")

}

if len(artifactTypematch) > 1 {

artifactType = artifactTypematch[1]

}

This can leverage the fact that the default value of golang string is "". Also since we are selecting only non-spaced characters we need not replace them. And since we are accessing the first element it is better to check for >1 as length.

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

ashokponkumar · 2020-10-19T15:43:49Z

Thanks for the nice work with this PR @isabellaliu77 ! Feel free to submit more PRs for other issues, if you find time.

IsabellaDev · 2020-10-19T16:41:26Z

Glad I helped!

update to avoid hard coding of the artifactType

d3d5d74

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

IsabellaDev force-pushed the master branch from 3f8bd91 to d3d5d74 Compare October 17, 2020 02:58

ashokponkumar requested changes Oct 17, 2020

View reviewed changes

update to remove any hardcoding, and remove possible whitespaces

5c42e08

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

IsabellaDev force-pushed the master branch from f85a562 to 5c42e08 Compare October 18, 2020 05:11

IsabellaDev requested a review from ashokponkumar October 18, 2020 05:15

ashokponkumar reviewed Oct 18, 2020

View reviewed changes

updated regexp groups to fetch artifactType

cd2e1d4

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

IsabellaDev force-pushed the master branch from bc8fc6a to cd2e1d4 Compare October 19, 2020 02:02

ashokponkumar requested changes Oct 19, 2020

View reviewed changes

simplified the logic of setting the value for artifactType

bfd0eb4

Signed-off-by: Isabella Liu <isabellaliu77@gmail.com>

IsabellaDev force-pushed the master branch from 5616e41 to bfd0eb4 Compare October 19, 2020 15:06

IsabellaDev requested a review from ashokponkumar October 19, 2020 15:07

ashokponkumar approved these changes Oct 19, 2020

View reviewed changes

ashokponkumar merged commit 5c2e964 into konveyor:master Oct 19, 2020

ashokponkumar linked an issue Oct 19, 2020 that may be closed by this pull request

Use regex substring to get portion of string #27

Closed

HarikrishnanBalagopal added the kind/bug Categorizes issue or PR as related to a bug. label Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update to avoid hard coding of the artifactType #30

update to avoid hard coding of the artifactType #30

IsabellaDev commented Oct 17, 2020

ashokponkumar Oct 17, 2020

IsabellaDev Oct 18, 2020

ashokponkumar Oct 17, 2020

IsabellaDev Oct 18, 2020

ashokponkumar Oct 18, 2020

ashokponkumar Oct 17, 2020

IsabellaDev Oct 18, 2020

ashokponkumar Oct 18, 2020

ashokponkumar commented Oct 17, 2020

ashokponkumar Oct 18, 2020

IsabellaDev Oct 18, 2020

ashokponkumar Oct 18, 2020

IsabellaDev Oct 19, 2020

ashokponkumar Oct 19, 2020

IsabellaDev Oct 19, 2020

ashokponkumar Oct 19, 2020

ashokponkumar commented Oct 19, 2020

IsabellaDev commented Oct 19, 2020

	re := regexp.MustCompile(`artifactType:\s*(.+)`)
	re := regexp.MustCompile(`artifactType:\s*([^\s]+)`)

update to avoid hard coding of the artifactType #30

update to avoid hard coding of the artifactType #30

Conversation

IsabellaDev commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokponkumar commented Oct 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashokponkumar commented Oct 19, 2020

IsabellaDev commented Oct 19, 2020