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

🌱 :*: use 'logicalCluster' instead of 'this' #2524

Merged

Conversation

stevekuznetsov
Copy link
Contributor

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

Script used to generate this
commit 226d0d24566680c029c1ccb558021e815d7d0071
Author: Steve Kuznetsov <skuznets@redhat.com>
Date:   Wed Dec 21 07:22:28 2022 -0700

    automated refactor

diff --git a/go.mod b/go.mod
index dcad2074..98cf850f 100644
--- a/go.mod
+++ b/go.mod
@@ -9,6 +9,7 @@ require (
 	github.com/bombsimon/logrusr/v3 v3.0.0
 	github.com/coredns/caddy v1.1.1
 	github.com/coredns/coredns v1.9.3
+	github.com/dave/dst v0.27.2
 	github.com/egymgmbh/go-prefix-writer v0.0.0-20180609083313-7326ea162eca
 	github.com/emicklei/go-restful v2.9.5+incompatible
 	github.com/evanphx/json-patch v5.6.0+incompatible
@@ -34,6 +35,7 @@ require (
 	go.etcd.io/etcd/server/v3 v3.5.0
 	go.uber.org/multierr v1.7.0
 	golang.org/x/net v0.0.0-20220722155237-a158d28d115b
+	golang.org/x/tools v0.1.12
 	gopkg.in/square/go-jose.v2 v2.2.2
 	k8s.io/api v0.24.3
 	k8s.io/apiextensions-apiserver v0.24.3
@@ -162,7 +164,6 @@ require (
 	golang.org/x/term v0.0.0-20210927222741-03fcf44c2211 // indirect
 	golang.org/x/text v0.3.7 // indirect
 	golang.org/x/time v0.0.0-20220210224613-90d013bbcef8 // indirect
-	golang.org/x/tools v0.1.12 // indirect
 	gonum.org/v1/gonum v0.6.2 // indirect
 	google.golang.org/appengine v1.6.7 // indirect
 	google.golang.org/genproto v0.0.0-20220519153652-3a47de7e79bd // indirect
diff --git a/go.sum b/go.sum
index 8b906ef0..4a8f683a 100644
--- a/go.sum
+++ b/go.sum
@@ -194,6 +194,9 @@ github.com/creack/pty v1.1.11 h1:07n33Z8lZxZ2qwegKbObQohDhXDQxiMMz1NOUGYlesw=
 github.com/creack/pty v1.1.11/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/cyphar/filepath-securejoin v0.2.3 h1:YX6ebbZCZP7VkM3scTTokDgBL2TY741X51MTk3ycuNI=
 github.com/cyphar/filepath-securejoin v0.2.3/go.mod h1:aPGpWjXOXUn2NCNjFvBE6aRxGGx79pTxQpKOJNYHHl4=
+github.com/dave/dst v0.27.2 h1:4Y5VFTkhGLC1oddtNwuxxe36pnyLxMFXT51FOzH8Ekc=
+github.com/dave/dst v0.27.2/go.mod h1:jHh6EOibnHgcUW3WjKHisiooEkYwqpHLBSX1iOBhEyc=
+github.com/dave/jennifer v1.5.0 h1:HmgPN93bVDpkQyYbqhCHj5QlgvUkvEOzMyEvKLgCRrg=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -690,8 +693,8 @@ github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQD
 github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
 github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
 github.com/seccomp/libseccomp-golang v0.9.2-0.20210429002308-3879420cc921/go.mod h1:JA8cRccbGaA1s33RQf7Y1+q9gHmZX1yB/z9WDN1C6fg=
-github.com/sergi/go-diff v1.1.0 h1:we8PVUC3FE2uYfodKH/nBHMSetSfHDR6scGdBi+erh0=
 github.com/sergi/go-diff v1.1.0/go.mod h1:STckp+ISIX8hZLjrqAeVduY0gWCT9IjLuqbuNXdaHfM=
+github.com/sergi/go-diff v1.2.0 h1:XU+rvMAioB0UC3q1MFrIQy4Vo5/4VsRDQQXHsEya6xQ=
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.4.1/go.mod h1:ni0Sbl8bgC9z8RoU9G6nDWqqs/fq4eDPysMBDgk/93Q=
diff --git a/refactor.go b/refactor.go
new file mode 100644
index 00000000..7f7b6bef
--- /dev/null
+++ b/refactor.go
@@ -0,0 +1,174 @@
+/*
+Copyright 2022 The KCP Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package main
+
+import (
+	"errors"
+	"flag"
+	"fmt"
+	"go/ast"
+	"go/types"
+	"os"
+	"path/filepath"
+
+	"github.com/dave/dst"
+	"github.com/dave/dst/decorator"
+	"github.com/dave/dst/decorator/resolver/gopackages"
+	"github.com/dave/dst/dstutil"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/tools/go/packages"
+)
+
+type options struct {
+	packageNames []string
+}
+
+func newOptions() *options {
+	o := &options{}
+	return o
+}
+
+func (o *options) addFlags(fs *flag.FlagSet) {
+}
+
+func (o *options) complete(args []string) error {
+	o.packageNames = args
+	return nil
+}
+
+func (o *options) validate() error {
+	if len(o.packageNames) == 0 {
+		return errors.New("packages to scan are required arguments")
+	}
+	return nil
+}
+
+func main() {
+	o := newOptions()
+	o.addFlags(flag.CommandLine)
+	flag.Parse()
+	if err := o.complete(flag.Args()); err != nil {
+		logrus.WithError(err).Fatal("could not complete options")
+	}
+	if err := o.validate(); err != nil {
+		logrus.WithError(err).Fatal("invalid options")
+	}
+	if err := rewrite(o.packageNames); err != nil {
+		logrus.WithError(err).Fatal("failed to re-write source")
+	}
+}
+
+func rewrite(packageNames []string) error {
+	pkgs, err := decorator.Load(&packages.Config{
+		Mode: packages.NeedName | packages.NeedFiles | packages.NeedCompiledGoFiles |
+			packages.NeedImports | packages.NeedTypes | packages.NeedTypesSizes |
+			packages.NeedSyntax | packages.NeedTypesInfo | packages.NeedModule,
+		Tests: true,
+	}, packageNames...)
+	if err != nil {
+		return fmt.Errorf("failed to load source: %w", err)
+	}
+	for _, pkg := range pkgs {
+		restorer := decorator.NewRestorerWithImports(pkg.PkgPath, gopackages.New(""))
+		fileRestorer := restorer.FileRestorer()
+		for i, file := range pkg.Syntax {
+			filePath := pkg.CompiledGoFiles[i]
+			dir := pkg.Dir
+			if pkg.Module != nil { // even though we ask for modules, if we run on a single file we don't get them
+				dir = pkg.Module.Dir
+			}
+			relPath, err := filepath.Rel(dir, filePath)
+			if err != nil {
+				return fmt.Errorf("should not happen: could not find relative path to %s from %s", filePath, pkg.Dir)
+			}
+
+			logrus.WithFields(logrus.Fields{"file": relPath}).Info("Considering file.")
+			var updates int
+			mutatedNode := dstutil.Apply(file, nil, func(cursor *dstutil.Cursor) bool {
+				for _, update := range []func(pkg *decorator.Package, cursor *dstutil.Cursor, fileRestorer *decorator.FileRestorer) int{
+					rewriteThisName,
+				} {
+					updates += update(pkg, cursor, fileRestorer)
+				}
+				return true
+			})
+			if updates > 0 {
+				logrus.WithFields(logrus.Fields{"file": relPath, "updates": updates}).Info("Updating file.")
+				previous, err := os.ReadFile(filePath)
+				if err != nil {
+					return fmt.Errorf("failed to read contents of %s: %w", filePath, err)
+				}
+				f, err := os.OpenFile(filePath, os.O_RDWR|os.O_TRUNC, 0666)
+				if err != nil {
+					return fmt.Errorf("failed to open %s for writing: %w", relPath, err)
+				}
+				if err := fileRestorer.Fprint(f, mutatedNode.(*dst.File)); err != nil {
+					if err := os.WriteFile(filePath, previous, 0666); err != nil {
+						logrus.WithFields(logrus.Fields{"file": relPath}).WithError(err).Error("Failed to restore contents of file.")
+					}
+					return fmt.Errorf("failed to write %s: %w", relPath, err)
+				}
+			}
+		}
+	}
+	return nil
+}
+
+func rewriteThisName(pkg *decorator.Package, cursor *dstutil.Cursor, fileRestorer *decorator.FileRestorer) int {
+	var updated int
+	switch node := cursor.Node().(type) {
+	case *dst.Ident:
+		if node.Name != "this" {
+			break
+		}
+		astNode, ok := pkg.Decorator.Ast.Nodes[node]
+		if !ok {
+			logrus.Info("no astNode")
+			break
+		}
+		astIdent, ok := astNode.(*ast.Ident)
+		if !ok {
+			logrus.Infof("astNode not an ident: %T", astNode)
+			break
+		}
+		var objType types.Object
+		def, ok := pkg.TypesInfo.Defs[astIdent]
+		if ok && def != nil {
+			objType = def
+		}
+
+		use, ok := pkg.TypesInfo.Uses[astIdent]
+		if ok && use != nil {
+			objType = use
+		}
+
+		if objType == nil {
+			break
+		}
+
+		if node.Name == "this" && objType.Type().String() == "*github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1.LogicalCluster" {
+			cursor.Replace(&dst.Ident{
+				Name: "cluster",
+				Path: node.Path,
+				Obj:  node.Obj,
+				Decs: node.Decs,
+			})
+			updated += 1
+		}
+	}
+	return updated
+}

@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Dec 21, 2022
@stevekuznetsov stevekuznetsov changed the title *: use 'cluster' instead of 'this' 🌱 :*: use 'cluster' instead of 'this' Dec 21, 2022
@stevekuznetsov
Copy link
Contributor Author

Ah of course not so easy when other identifiers in the scope already are called cluster :|

@hardys
Copy link

hardys commented Dec 22, 2022

Ah of course not so easy when other identifiers in the scope already are called cluster :|

Maybe using logicalCluster instead will avoid that collision? As someone with C++ in my past, I agree we should rename away from this though :)

@sttts
Copy link
Member

sttts commented Dec 23, 2022

Sgtm. I used logicalCluster already in a number of places.

1 similar comment
@sttts
Copy link
Member

sttts commented Dec 23, 2022

Sgtm. I used logicalCluster already in a number of places.

Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@stevekuznetsov stevekuznetsov changed the title 🌱 :*: use 'cluster' instead of 'this' 🌱 :*: use 'logicalCluster' instead of 'this' Jan 3, 2023
@stevekuznetsov
Copy link
Contributor Author

/retest

@stevekuznetsov stevekuznetsov added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. labels Jan 3, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 3, 2023

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit bf532ce into kcp-dev:main Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants