Go implementation of Piranha by crekIron · Pull Request #112 · uber/piranha

crekIron · 2021-03-30T17:29:30Z

Piranha implementation for Golang. See README.md for more info.

CLAassistant · 2021-03-30T17:29:40Z

All committers have signed the CLA.

mkr-plse · 2021-03-31T13:32:48Z

@crekIron Thanks for the PR and for extending Piranha for Go. Can you please sign the CLA so that the PR can be reviewed? Thanks!

crekIron · 2021-04-01T07:04:17Z

I did it.

mkr-plse · 2021-04-01T12:53:39Z

I did it.

Seems signed now. Thanks!

lazaroclapp

Hey @crekIron, thanks for the PR!

This is really cool, and I appreciate the test suite included (let's hook it to CI before landing this!). I did have a number of comments on the PR, and a few things that aren't quite clear to me. See below. It's just a first pass, but hopefully it's pretty detailed!

I also think there are parts that would benefit from someone with more experience with Go parsing took a look too (cc: @awelc )

lazaroclapp · 2021-04-05T19:23:25Z

+Optional arguments:
+		-h: Show the options and exit.
+		-treated: If this is given, then flag is treated,
+			otherwise it is control.


Do we want control to be implicit? I believe most other Piranha versions take an explicit string, which is either treatment or control (and, for some languages, and probably for Go in the future, they can also take a specific named treatment group).

We can make it different. But I thought making it implicit is still fine. I think it's generally less pain to first-time users as they have to set fewer attributes initially when they are trying out. (I will answer other comments slowly and steadily:)

My thinking here is that, since we are deleting code in place, we do want the user to be somewhat explicit about which logic ought to be deleted, even if it's a bit more friction. It's also not clear that one default would be more common than the other.

Hmm, I agree with you. I suggest we can ask for as like:
-mode treated or -mode control. Is it fine?

Sure. That makes sense to me :)

lazaroclapp · 2021-04-05T19:24:34Z

+			otherwise it is control.
+		-o OUTPUT: Destination of the refactored output from piranha. If -o is not provided, then the source file is updated in place.
+```
+3. To do a test run, run piranha on `example/testExample.go`. Run `./piranha -p properties.json -s ./example/testExample.go -o ./example/treatedExample.go -f staleFlag` command in root directory. You will get your refracted file as `/example/treatedExample.go`.


"refactored", not "refracted", I assume.

I will change that.

lazaroclapp · 2021-04-05T19:26:04Z

+        "argumentIndex": 1
+      }
+    ]
+  }


Lets have files end in a newline unless we have a really good reason not to.

Ok, I will do that. But like any specific reason for it? Just curious.

Ending files with new line is considered best practice. One reason I know of is because command line tools might otherwise merge the last and first line of two files when processing a list of files. Also, some programs will fail to parse the last line entirely if it doesn't end in newline (not that bizarre behavior, actually, since POSIX does define a line as "A sequence of zero or more non- characters plus a terminating character.")

lazaroclapp · 2021-04-05T19:29:31Z

+			}
+		}
+		if FileNotMatched {
+			t.Errorf("Files didn't matched, see above output")


Should be: "Files didn't match,"

Yup will fix it.

lazaroclapp · 2021-04-05T19:31:07Z

@@ -0,0 +1,4 @@
+# Testing of PiranhaGo
+1. Run `go test` in this directory to test the working of Piranha.


I love that this has such a comprehensive test suite!

That said, let's get it hooked with CI from day one. Shouldn't be a ton more work, and would reassure us that this tests run correctly now and keep running correctly as changes to this PR and future PR are made :)

How to do that? Do you mean github cli?

I mean CI as in "Continuous Integration" (not CLI). GitHub's CI is indeed what I'd recommend (see https://github.com/features/actions ). I believe we are already using that for PiranhaJS.

Note: PiranhaJava and Swift are on Travis CI for now. But I would not recommend new projects to use Travis CI.

lazaroclapp · 2021-04-05T21:55:53Z

+				if valY == isFalse && d.Op != token.LAND {
+					n.Replace(d.X)
+				}
+				if d.Op == token.EQL {


Given the cases above, and EQL not being either LOR or LAND, isn't this unreachable? (I am also not sure the conditionals above are safe for !=. Do we have an exhaustive list of binary ops valid for at least one boolean operand for Go?)

lazaroclapp · 2021-04-05T22:00:51Z

+				break
+			}
+		}
+	}


Do we need this? Wouldn't it be better to just check for the argument at element.FlagIndex below, right above line 152, to see if it matches sfc.flagName and skip otherwise?

Yeah, we don't need it. Dunno, why I didn't seen it.

lazaroclapp · 2021-04-05T22:03:30Z

+					return isFalse
+				}
+			case token.EQL:
+				if valX != isBot && valY != isBot {


Technically subsumed by the check for valX != isBot || valY != isBot above, I think...

Actually, when one of them is undefined, then I need to return isUndefined for the token.EQL case. It is getting returned at the last.

lazaroclapp · 2021-04-05T22:06:11Z

+				return isTrue
+			}
+		case token.AND: // &expr
+			return sfc.evaluateExprNode(exprUnderConsideration.X)


Uh? What is this case? If this is actually for &expr, it doesn't seem correct to me, since taking the address of a boolean value (e.g. &true not a *bool) just gives you an error.

I am not sure how this works either. Since we simply return X here, don't we equate expr with &expr? What is the reasoning behind it (similar question for the StarExpr)?

Take an example (I know, it's very rare).....

staleVariable := funcAffectedByStaleFlag(.....) ...... pointerToVar := &staleVariable // getting pointer to staleVariable a := *pointerToVar && false

To handle the below two expressions I did it. I tried extracting the value of the variable from 2nd expression and store it in the valueMap which can be used to handle the 3rd expression.

lazaroclapp · 2021-04-05T22:07:13Z

+		}
+	//Star expression (*expr)
+	case *dst.StarExpr:
+		return sfc.evaluateExprNode(exprUnderConsideration.X)


Again, *true is not a bool, it's an error. Am I missing something here?

check above comment.

awelc · 2021-04-14T23:45:12Z

+	var sourceFile, configFile, flagName, outputFileName string = "", "", "", ""
+	var isTreated = false
+	sizeOfArgs := len(inArgs)
+	if len(inArgs) < 2 {


I am not sure why you parse the arguments by hand. Unless there is a really good reason, please use the flag package (or a different one) that standardizes it.

Ok, I will use flag one.

awelc · 2021-04-14T23:49:16Z

+	"github.com/dave/dst/dstutil"
+)
+
+// API : For the type of API


According to our coding conventions, comments for exported entities should start with their name (as they currently are actually) and be full sentences ending with a dot. For example here it could be API describes type of the API.

awelc · 2021-04-14T23:49:46Z

+// API : For the type of API
+type API int
+
+const (


Private constant names should start with _.

awelc · 2021-04-14T23:50:58Z

+type Operator int
+
+const (
+	and Operator = iota


awelc · 2021-04-14T23:54:36Z

+)
+
+// Some useful global variable
+var treated, control string = "treated", "control"


Stylistically, it would look better as (grouping related variables and no need to specify the type, particularly that in this case it's obvious what it is):

var ( treated = "treated" control = "control" )

Yeah sure. This looks good.

awelc · 2021-04-15T00:35:32Z

+			}
+		}
+	//declstmt
+	case *dst.DeclStmt:


Why do we handle it here instead of handling it when processing GenDecl above?

Two reasons:

DeclStmt can occur anywhere else too.

Tree traversal is happening. So, children of GenDecl is going to get checked anyway.

awelc · 2021-04-15T00:35:46Z

+		// Init of Ifstmt will get handled in assignStmt
+		// Now evaluate the cond. Cond is already refactored due to pre-order traversal.
+		cond := sfc.evaluateExprNode(d.Cond)
+		if cond != isBot {


awelc · 2021-04-15T00:37:01Z

+						valCaseClause := sfc.evaluateExprNode(exprstmt.(*dst.CaseClause).List[0])
+						if valCaseClause == valExpr {
+							//this has to go out as switch is going to delete
+							for _, bodyEle := range exprstmt.(*dst.CaseClause).Body {


Is exprstmt always of the CaseClause type? If so, then why isn't it of the CaseClause type in the first place? Do we need a type check here instead of type assertion?

awelc · 2021-04-15T00:37:45Z

+							//this has to go out as switch is going to delete
+							for _, bodyEle := range exprstmt.(*dst.CaseClause).Body {
+								n.InsertBefore(bodyEle)
+							}


It certainly is possible.

awelc · 2021-04-15T00:40:41Z

+	return true
+}
+
+// This is the Upper flow of the programme


According to our style guide, you order your file (roughly) in the calling order. It makes sense in general, as it improves readability - you start with the "main" part and look for the helper functions as you keep parsing the code, rather than the other way around, that is starting with looking at helper functions.

sandyskies · 2022-04-12T08:44:15Z

@lazaroclapp @crekIron Can u guys flow up this pull request? We need this pull request to clean up Go feature-flag. Appreciate！

mkr-plse · 2022-04-15T05:43:31Z

@lazaroclapp @crekIron Can u guys flow up this pull request? We need this pull request to clean up Go feature-flag. Appreciate！

@sandyskies We are working on a re-architected implementation of Piranha using TreeSitter where onboarding for different languages is made simpler and faster. We will be able to release a version in the next month or so, and enabling Piranha for Go will be much easier. cc @ketkarameya.

crekIron · 2022-04-19T17:23:43Z

I would like to complete this pull request. I have nearly handled all the comments.

Leoyoungxh · 2022-05-11T07:35:06Z

+	flag.StringVar(&flagName, "f", "STALE_FLAG", "Name of the stale flag.")
+	// check treatedMode
+	flag.StringVar(&modeName, "mode", "MODE_NAME", "If MODE_NAME=treated, then flag is treated, otherwise MODE_NAME=control, it is control.")
+	if modeName == "treated" {


It should be excuted after L40 flag.Parse().

Thanks for pointing it out. I changed it now.

…. Testing works both linux and windows

crekIron added 2 commits March 30, 2021 22:48

making folder go and transferring code to it

ac2707f

added link of go readme

7ac72d1

mkr-plse requested review from lazaroclapp and robbertvanginkel March 30, 2021 19:01

mkr-plse mentioned this pull request Mar 30, 2021

Golang support #71

Open

mkr-plse linked an issue Mar 30, 2021 that may be closed by this pull request

Golang support #71

Open

lazaroclapp suggested changes Apr 5, 2021

View reviewed changes

lazaroclapp requested a review from awelc April 5, 2021 22:13

awelc reviewed Apr 15, 2021

View reviewed changes

mkr-plse requested a review from awelc April 16, 2021 14:36

crekIron and others added 8 commits June 1, 2021 19:27

changes in readme and test file

36445bf

Some changes done.

38274f7

Merge branch 'uber:master' into master

fbbc08c

Used flag package to parse the arguments.

47b1f0e

Changes made in src/piranha.go

38680bd

Incoperated 30% of comments in src/staleFlagCleaner.go

b37339e

Incoperated 70% of comments in src/staleFlagCleaner.go

6ff77de

Merge branch 'uber:master' into master

3760732

Leoyoungxh reviewed May 11, 2022

View reviewed changes

Fixed logical error in main.go. Resolves issues in testing on Windows…

448cabc

…. Testing works both linux and windows

ketkarameya force-pushed the master branch from d802f3e to 3955145 Compare September 7, 2023 02:55

		@@ -0,0 +1,4 @@
		# Testing of PiranhaGo
		1. Run `go test` in this directory to test the working of Piranha.

Conversation

crekIron commented Mar 30, 2021

Uh oh!

CLAassistant commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkr-plse commented Mar 31, 2021

Uh oh!

crekIron commented Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkr-plse commented Apr 1, 2021

Uh oh!

lazaroclapp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crekIron Apr 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crekIron Aug 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Mar 30, 2021 •

edited

Loading

crekIron commented Apr 1, 2021 •

edited

Loading

crekIron Apr 4, 2022 •

edited

Loading

crekIron Aug 7, 2021 •

edited

Loading