Dev #12

bharathi-tenneti · 2019-10-31T14:32:35Z

Code review changes made, by adding below
--RepoMetadataQuertry struct, and Version Parsser interface.
--Added ERR check for GetStats func call in main.go
--Added skiVersion Flag to skip ParseVersion func calls when Query contains
Gopkg.toml"

getVersion func modified to remove loop

Code Review changes made, VersionParser Interface introduced

parseVersion() fixed

Dev

joelanford

Looks like a good set of improvements from last time. Here's another round or comments and suggestions. We're getting there! 👍

joelanford · 2019-10-31T20:21:02Z

cmd/sdkstats/main.go

+		} else if _, ok := err.(*github.RateLimitError); ok {
+			log.Println("Rate Limit has reached.")
+		} else if err != nil {
+			fmt.Println("Failed to get Stats for Query", r, err)


This is a great opportunity to use a type switch. I'd suggest using this instead of if .. else if blocks.

joelanford · 2019-10-31T20:22:01Z

cmd/sdkstats/main.go

-			_ = ioutil.WriteFile(fileName, file, 0644)
-			fmt.Println("Results are written in ", fileName)
+		if _, ok := err.(*github.AcceptedError); ok {
+			log.Println("Job is scheduled on GitHub side")


Is this error something we can recover from? If something is scheduled on the Github side, can we make a follow-up request to check to see if the results are ready?

I'd also suggest including the underlying error string in the message since it might help users understand the issue more.

I thought about it, again we need to keep process running till we wait. Also, log.Println("Job is scheduled on GitHub side"), this string is coming from Library example of handling the error., Please see this README, https://github.com/google/go-github

From the github client README about the Accepted Error:

Methods known to behave like this are documented specifying this behavior.

We should check to see if the methods we're calling can emit this error. If so, we should look into what it means and what (if any) follow-up calls we can make to get the data one the processing on the github side has completed.

joelanford · 2019-10-31T20:34:44Z

cmd/sdkstats/main.go

+		if _, ok := err.(*github.AcceptedError); ok {
+			log.Println("Job is scheduled on GitHub side")
+		} else if _, ok := err.(*github.RateLimitError); ok {
+			log.Println("Rate Limit has reached.")


Ditto about printing the error string from the rate limit error.

Also, log.Println("Rate Limit has reached."), this string is coming from Library example of handling the error. Please see below link https://github.com/google/go-github README

I think those are meant to be simple examples. You can see the actual error text in the code here.

When printing an error, it is conventional to add some context and then print the underlying error, like this:

fmt.Printf("Could not fetch GitHub stats: %v\n", err)

Until we look into how to handle the AcceptedError, we should do something similar for that, IMO.

Yes, Will include the original ERR. Also, On that note, I am still getting the same error. Will investigate more.

joelanford · 2019-10-31T20:37:08Z

cmd/sdkstats/main.go

+		fileName := r.ProjectType + ".json"
+		fmt.Println("Total count: ", len(stats))
+		file, _ := json.MarshalIndent(stats, "", " ")
+		_ = ioutil.WriteFile(fileName, file, 0644)
+		fmt.Println("Results are written in ", fileName)


Rather than doing a file per project type, it would be nice to combine everything and print it to standard out. Then, if users want it in a file, they can always pipe it into one.

Maybe we update GetStats() to accept the entire slice of queries and return the combined results as a flat slice. Then we can iterate over the returned results to compute our final stats. How does that sound?

As fas as main.go goes, I took our requirements into consideration, as we were talking about ProjectType based breakdown, I have written into corresponding files.

Please see my above note, and let me know what you think

Yeah I think we miscommunicated a little about ProjectType breakdown. I just meant that one of the stats that would be interesting is the number of projects of each project type, not that we should split stats into separate files. Sorry about that!

No Problem, this has been evolving anyways, I can make that change.

joelanford · 2019-10-31T20:49:36Z

cmd/sdkstats/main.go

 }
+
+//Parse the given Code result to search Text Matches for Version number.
+func (p projectVersionParser) ParseVersion(codeResults github.CodeResult, projectType string) (string, error) {


I don't think you need projectType in the function signature, since you're passing an implementation of this into GetStats as part of a RepoMetadataQuery that already includes the project type.

I would suggest first writing a separate implementation of this interface for each query, and then looking to see if there are ways to re-use.

For example, the helm and ansible version parser implementations are basically identical other than a certain anchor you're looking for (e.g. the image name). So maybe you end up with something like this:

type baseImageVersionParser struct { baseImage string } ansibleVersionParser := baseImageVersionParser{baseImage: "quay.io/operator-framework/ansible-operator"} helmVersionParser := baseImageVersionParser{baseImage: "quay.io/operator-framework/helm-operator"}

For the Go projects, maybe it makes sense to have a goModVersionParser and a gopkgTomlVersionParser, where you would end up making separate queries to use each of them. Then you could use your existing implementation for go.mod and then have the Gopkg.toml version parser just return "unknown", nil or similar.

I am not sure., I completely understand this. Since "Gopkg.toml" results , would not need ParseVersion, as they do not have defined search Query, I am defaulting sdkVersion to "N/A". Hence, I thought its unnecessary to make function calls to ParseVersion from sdkstats.go for below condition , if strings.Contains(q, "Gopkg.toml") {
skipVersion = 1
}

"I don't think you need projectType in the function signature, since you're passing an implementation of this into GetStats as part of a RepoMetadataQuery that already includes the project type."
So, You are saying its better to have separate ParseFunction defined, rather than passing "ProjectType", from sdkstats? Or May be I use which is available in Main.go, Not sure if that thread will still give me current ProjectType value passed into sdkstats.go.

Think of it from a library perspective and pretend you don't know what queries other users will be sending in. In that case, all the library can do is use the provided version parser.

So then from our perspective in main.go, think of things in terms of the API we have available, which is the VersionParser interface. In the case of Gopkg.toml, we're unable to parse a version so we define a version parser whose sole job is to return "unknown", nil.

type unknownVersionParser struct{} func (_ unknownVersionParser) ParseVersion(_ github.CodeResult) (string, error) { return "unknown", nil }

Since that means we have different implementations for parsing go.mod and Gopkg.toml, it probably means we need to split our ProjectType: "go" queries into two ProjectType: "go" queries like this:

queries := []sdkstats.RepoMetadataQuery{ sdkstats.RepoMetadataQuery{ ProjectType: "ansible", Queries: []string{"filename:Dockerfile quay.io/operator-framework/ansible-operator"}, VersionParser: &baseImageVersionParser{ baseImage: "quay.io/operator-framework/ansible-operator", }, }, sdkstats.RepoMetadataQuery{ ProjectType: "helm", Queries: []string{"filename:Dockerfile quay.io/operator-framework/helm-operator"}, VersionParser: &baseImageVersionParser{ baseImage: "quay.io/operator-framework/helm-operator", }, }, sdkstats.RepoMetadataQuery{ ProjectType: "go", Queries: []string{ "filename:go.mod github.com/operator-framework/operator-sdk", }, VersionParser: &goModVersionParser{ module: "github.com/operator-framework/operator-sdk", }, }, sdkstats.RepoMetadataQuery{ ProjectType: "go", Queries: []string{ "filename:Gopkg.toml github.com/operator-framework/operator-sdk", }, VersionParser: &unknownVersionParser{}, }, }

I agree with separating the queries for "go.mod", and "Gopkg.toml". I was having them as separate queries in the beginning as well, to make VersionParse easier. Will make the changes.

joelanford · 2019-10-31T21:02:32Z

pkg/sdkstats/sdkstats.go

+		for j := 0; j < len(searchOp); j++ {
+			for i := 0; i < len(searchOp[j].CodeResults); i++ {
+				if skipVersion == 1 {
+					sdkVersion = "N/A"


This is starting to come into shape as a general purpose library, so should we use version as the variable name?

I dont think I understood this, Can you please elaborate?

Sorry, I'm just saying, lets change the variable name from "sdkVersion" to "version" now that this library his become much more extensible and usable for totally different projects.

joelanford · 2019-10-31T21:16:21Z

pkg/sdkstats/sdkstats.go

-
-	repoMap := map[string]string{}
-
+//GetRepoDetails Returns []RepoMetaData with Stars,CreatedAt,PushedAt, and TotalCommits details for a given owner, and repo


A couple of nits, not super important, but good to instill good habits (this is as much a reminder for me as is it for anyone else reading this 🙂)

Include a space after the comment marker //

The first word of a comment that describes an identifier should be the identifier, so good to go there 👍

Use full sentences that end with a period

It's documentation for humans, so write what seems most natural and helpful.

With that in mind, perhaps this:

Suggested change

//GetRepoDetails Returns []RepoMetaData with Stars,CreatedAt,PushedAt, and TotalCommits details for a given owner, and repo

// GetRepoDetails uses client to look up repository metadata about repo.

// The returned RepoMetadata struct includes details such as the

// repository's stars, timestamps when it was created and most recently

// pushed to, and the total number of commits to the repo. If an error

// occurs, *RepoMetadata will be nil, and the error will be non-nil.

Sounds reasonable, will include.

Another point is that it is most important to pay attention to the doc conventions on exported functions. Since that's what users of the library will see.

Comments on unexported functions are still nice, but are only helpful to other developers and maintainers of the library.

joelanford · 2019-10-31T21:17:53Z

pkg/sdkstats/sdkstats.go

 	ctx := context.Background()
-
-	repos, _, err := client.Repositories.Get(ctx, owner, name)
+	repos, _, err := client.Repositories.Get(ctx, repoDetails.Owner, repoDetails.Name)


Nit: Should repos be plural or singular? It seems like this function returns one repo?

:D Yes, It seems good to name it repo.

joelanford · 2019-10-31T21:19:35Z

pkg/sdkstats/sdkstats.go

-
+//GetRepoDetails Returns []RepoMetaData with Stars,CreatedAt,PushedAt, and TotalCommits details for a given owner, and repo
+func GetRepoDetails(client *github.Client, repoDetails *RepoMetadata) (*RepoMetadata, error) {
 	ctx := context.Background()


In library code, it's best to let users pass their own context. The convention is to make the first argument in exported functions be the context, so this would be:

func GetRepoDetails(ctx context.Context, client *github.Client, repoDetails *RepoMetadata) (*RepoMetadata, error) {

Since we call GetRepoDetails from another function in this library, we should also update that function's signature as well, and so on, until we reach a function that is not called anywhere else.

joelanford · 2019-10-31T21:29:32Z

pkg/sdkstats/sdkstats.go

-				return nil, err
-			}
+//GetStats returns []RepoMetaData populated with Code results from Github Search Code API, for a given search string
+func GetStats(client *github.Client, rq RepoMetadataQuery) ([]RepoMetadata, error) {


This is the only function used in main.go, so that's probably a good indication that the other functions (e.g. Search and GetRepoDetails) do not need to be exported and made public. To unexport the other functions, simply make the the first letter of their name lowercase.

An extremely important aspect of maintaining libraries is to be very careful and deliberate about designing and changing their public API surface. Any breaking changes to a library make life more difficult for users of the library, so it's best to keep the API surface as small and intuitive as possible.

Later versions of libraries can always add new functionality and expose more parts of the API surface without breaking existing users, but going the opposite direction and making already public APIs private is a breaking change.

The reason I made these exportable, is that, In case other users have the need to let say only call "Search()", or "GetRepoDetails", It would be simple enough to call them. WDYT. This also makes case, NOT to pass "CodeResult" to GetRepoDetails(). WDYT?

Will make them unexportable based on this comment

See my comment here

joelanford · 2019-10-31T21:33:29Z

Looks like this PR has some conflicts with master as well. So we'll need to get those fixed up before merging too.

Code review changes for https://github.com/learnoperators/go-github-m…

Exception handling added in sdkstats.go

joelanford · 2019-11-05T16:42:32Z

cmd/sdkstats/main.go

 	"log"
+	"strings"

 	"github.com/go-github-metrics-1/pkg/sdkstats"


This line needs to match the actual repo path:

Suggested change

"github.com/go-github-metrics-1/pkg/sdkstats"

"github.com/learnoperators/go-github-metrics/pkg/sdkstats"

I am guessing I cant refer to this until I merge DEV with MASTER on learnoperators/go-github-metrics

I think you can. In your local environment, you can get this working in one of two ways:

Move the project directory into $GOPATH/src/github.com/learnoperators/go-github-metrics

Create go.mod file in the project root with go mod init github.com/learnoperators/go-github-metrics

I suggest option 2 since the SDK uses Go Modules and it is now the official dependency manager supported by Go's built-in tooling.

joelanford · 2019-11-05T17:00:46Z

cmd/sdkstats/main.go

-				fmt.Println(err)
+		stats, err := sdkstats.GetStats(ctx, tc, r)
+		if err != nil {
+			fmt.Printf("Failed to get stats for query  %q %v\n", r, err)


%q requires a string, not a struct. Now that r is a struct and the queries is a slice of strings, it would be better to use %v and r.Queries. Also the go convention is to always separate the context from the underlying error with a colon:

Suggested change

fmt.Printf("Failed to get stats for query %q %v\n", r, err)

fmt.Printf("Failed to get stats for queries %v: %v\n", r.Queries, err)

joelanford · 2019-11-05T17:03:12Z

cmd/sdkstats/main.go

+		if err != nil {
+			fmt.Printf("Failed to get stats for query  %q %v\n", r, err)
+		}
+		collectStats = append(collectStats, stats)


Any reason that we need a slice of slices? I think we can use this:

collectStats := []sdkstats.RepoMetadata{} ... collectStats = append(collectStats, stats...)

cannot use stats (type []sdkstats.RepoMetadata) as type sdkstats.RepoMetadata in appendgo
I see this error, when I try to make the change as suggested.

I think that means you need the ellipses after stats. This doc has some good examples: https://programming.guide/go/append-explained.html

joelanford · 2019-11-05T17:06:42Z

cmd/sdkstats/main.go

+	}
+	fileName := "Search_Results.json"
+	file, _ := json.MarshalIndent(collectStats, "", " ")
+	_ = ioutil.WriteFile(fileName, file, 0644)


ioutil.WriteFile returns an error. We need to make sure it returns nil or handle the error if not nil.

Ditto for MarshalIndent on the line above.

joelanford · 2019-11-05T18:48:13Z

cmd/sdkstats/main.go

 	)
 	tc := oauth2.NewClient(ctx, ts)
-	client := github.NewClient(tc)
+	//client := github.NewClient(tc)


I think a better solution here would be:

// Defined in pkg/sdkstats/sdkstats.go type Client struct { client *github.Client } func (c Client) GetStats(ctx context.Contect, queries []RepoMetadataQuery) ([]RepoMetadata, error) { // c.search(ctx, q) } func (c Client) search(ctx context.Context, query string) ([]*github.CodeSearchResult, error) { // c.Client.Search.Code(ctx, query, opts) }

And then in this file, you could do:

client := sdkstats.Client{Client: github.NewClient(tc)} stats, err := client.GetStats(ctx, queries)

Re-creating the client each time in search() and getRepoDetails() might actually have a negative performance impact.

joelanford · 2019-11-05T21:00:34Z

cmd/sdkstats/main.go

+				"filename:go.mod github.com/operator-framework/operator-sdk",
+			},
+			VersionParser: &gomodVersionParser{
+				searchQ: "replace github.com/operator-framework/operator-sdk => github.com/operator-framework/operator-sdk v",


We should not assume the SDK version will be in it's own replace statement. It could look like any of the following:

# Option 1: replace github.com/operator-framework/operator-sdk => github.com/operator-framework/operator-sdk <version> # Option 2 replace ( ... github.com/operator-framework/operator-sdk => github.com/operator-framework/operator-sdk <version> ... ) # Option 3 require ( ... github.com/operator-framework/operator-sdk <version> ... )

With these three options in mind, the pattern is:
<repo><whitespace><version><newline>

We can probably craft a regex for this as well.

joelanford · 2019-11-05T21:03:27Z

cmd/sdkstats/main.go


+// Parse the given Code result to search Text Matches for Version number.
+func (p unknownVersionParser) ParseVersion(codeResults github.CodeResult) (string, error) {
+	version := "N/A"


Nit: This isn't really "N/A" so much as "unknown" right? Gopkg.toml files DO have the SDK version in them, but we're just not able to parse it.

Also, there's no need to declare a variable and then return it. It's probably better to do return "unknown", nil.

joelanford · 2019-11-05T21:44:29Z

pkg/sdkstats/sdkstats.go

-				TotalCommits: TotalCommits,
+func GetStats(ctx context.Context, tc *http.Client, rq RepoMetadataQuery) ([]RepoMetadata, error) {
+	var repoList []RepoMetadata
+	var repoDetails *RepoMetadata


repoDetails doesn't need to be declared at this scope since it is only used in the loop.

In the loop, you can use the following to declare and initialize in a single statement:

repoDetails, err := getRepoDetails(...)

joelanford · 2019-11-05T21:49:23Z

pkg/sdkstats/sdkstats.go

+			for i := 0; i < len(searchOp[j].CodeResults); i++ {
+				repoDetails, err = getRepoDetails(ctx, tc, searchOp[j].CodeResults[i], rq)
+				if err != nil {
+					return repoList, err


This is an example of a situation where we may not want to exit the loop if one of the queries fails. If this function is a struct method on a new Client struct, we could add a Log function field to the struct and use that to log a message here:

type Client struct { ... Log func(format string, fields ...interface{}) (int, error) ... } repoDetails, err := getRepoDetails(...) if err != nil { c.Log("Failed to get repo details for %q: %v\n", searchOp[j].CodeResults[i].GetRepository().GetFullName(), err) // Keep processing the other results continue }

joelanford · 2019-11-05T21:54:17Z

pkg/sdkstats/sdkstats.go

 	}
-	if projectType == "go.mod" {
-		searchQ = "github.com/operator-framework/operator-sdk v0."
+	for _, r := range commits {


To make it clearer, declare totalCommits := 0 right above this loop rather than at the beginning of the function.

joelanford · 2019-11-05T22:15:49Z

pkg/sdkstats/sdkstats.go

+		}
+		for j := 0; j < len(searchOp); j++ {
+			for i := 0; i < len(searchOp[j].CodeResults); i++ {
+				repoDetails, err = getRepoDetails(ctx, tc, searchOp[j].CodeResults[i], rq)


It looks like we're calling getRepoDetails() more than once per repository, which unnecessarily hits the Github API, driving up the query count and making things hit the rate limit faster.

We should refactor things so that we only call getRepoDetails once per repo.

Since a repo might have more than one operator built into it, maybe we call need RepoMetadata to have a []string for Versions, and then we coul call getRepoDetails() once, and iterate the code results calling rq.VersionParser.ParseVersion() and appending to the Versions field.

WDYT?

joelanford

It sounds like you've made some changes (based on your "done" comments), but I don't see them in the review. Can you push these updates to the dev branch?

joelanford · 2019-11-11T15:52:09Z

cmd/sdkstats/main.go

 	"log"
+	"strings"

 	"github.com/go-github-metrics-1/pkg/sdkstats"


I think you can. In your local environment, you can get this working in one of two ways:

Move the project directory into $GOPATH/src/github.com/learnoperators/go-github-metrics

Create go.mod file in the project root with go mod init github.com/learnoperators/go-github-metrics

I suggest option 2 since the SDK uses Go Modules and it is now the official dependency manager supported by Go's built-in tooling.

Regex changes added

Added go.mod

joelanford · 2019-11-11T16:40:06Z

cmd/sdkstats/main.go

+// Parse the given Code result to search Text Matches for Version number.
+func (p baseVersionParser) ParseVersion(codeResults github.CodeResult) (string, error) {
+	baseImageRegex := regexp.QuoteMeta(p.searchQ)
+	versionRegex := strings.Trim(`(:([^s]+\n))?`, "\n")


I think you want the \n newline character to be appended after the version capture group, basically after the very end after the ?. Also don't think you need strings.Trim() here.

Added regex for gomodversionparser

camilamacedo86 · 2019-11-14T13:04:29Z

cmd/sdkstats/main.go

 	"golang.org/x/oauth2"
 )

+type helmVersionParser struct{}


As a good practice, we should have basic info in the README in order to allow any person knows how to configure using the project. It could be another PR, however, how we can review this one without the knowledge to test it locally?

camilamacedo86 · 2019-11-14T13:05:21Z

cmd/sdkstats/main.go

-	queries = append(queries, "filename:go.mod github.com/operator-framework/operator-sdk")
-	queries = append(queries, "filename:Gopkg.toml github.com/operator-framework/operator-sdk")
+	queries := []sdkstats.RepoMetadataQuery{
+		/*sdkstats.RepoMetadataQuery{


If the code is not useful, so could be removed. WDYT?

camilamacedo86 · 2019-11-14T13:51:33Z

cmd/sdkstats/main.go

+	searchLatest string
+}
+type gomodVersionParser struct {
+	searchQ string


Also, would be great have in the README an overview. As for example, describing the purpose of it. After I checked the code shows that we are looking for getting metrics over projects published in GitHub which were done using Operator-SDK. Am I right?

The starts, commits and etc are nor over operator-sdk repo/project. Am I right?

bharathi-tenneti added 9 commits October 29, 2019 15:57

getVersion func modified to remove loop

9e148a4

Merge pull request #7 from bharathi-tenneti/dev

3ad9d82

getVersion func modified to remove loop

Code Review changes made, VerrsionParser Interface introduced

db7d83f

Merge pull request #9 from bharathi-tenneti/dev

b3c425b

Code Review changes made, VersionParser Interface introduced

parseVersion() fixed

9338b6c

Merge pull request #10 from bharathi-tenneti/dev

058a8bd

parseVersion() fixed

Added skipVersion check, and err check in main.go

eac04fd

Comments removed for queries

c3267c0

Merge pull request #11 from bharathi-tenneti/dev

e8c3e35

Dev

joelanford suggested changes Oct 31, 2019

View reviewed changes

bharathi-tenneti added 4 commits November 4, 2019 11:51

Code review changes for #12 done

b40a7e3

Merge pull request #13 from bharathi-tenneti/dev

118c511

Code review changes for https://github.com/learnoperators/go-github-m…

Exception handling added in sdkstats.go

8e6c3f9

Merge pull request #14 from bharathi-tenneti/dev

3e192f8

Exception handling added in sdkstats.go

joelanford reviewed Nov 5, 2019

View reviewed changes

bharathi-tenneti added 2 commits November 6, 2019 22:52

PR review changes under progress. Total Counts added

c53fbc0

Client struct added

8bf7301

joelanford reviewed Nov 11, 2019

View reviewed changes

bharathi-tenneti and others added 4 commits November 11, 2019 10:56

Added REGEX logic for baseVersioParser implementation

f507210

Merge pull request #15 from bharathi-tenneti/dev

9568899

Regex changes added

Added go.mod

d496b79

Merge pull request #16 from bharathi-tenneti/dev

2f94656

Added go.mod

joelanford reviewed Nov 11, 2019

View reviewed changes

Bharathi Tenneti and others added 2 commits November 12, 2019 15:31

Added regex for gomodversionparser

23b00f9

Merge pull request #17 from bharathi-tenneti/dev

0ab9617

Added regex for gomodversionparser

camilamacedo86 reviewed Nov 14, 2019

View reviewed changes


		repoMap := map[string]string{}

		//GetRepoDetails Returns []RepoMetaData with Stars,CreatedAt,PushedAt, and TotalCommits details for a given owner, and repo

-//GetRepoDetails Returns []RepoMetaData with Stars,CreatedAt,PushedAt, and TotalCommits details for a given owner, and repo
+// GetRepoDetails uses client to look up repository metadata about repo.
+// The returned RepoMetadata struct includes details such as the
+// repository's stars, timestamps when it was created and most recently
+// pushed to, and the total number of commits to the repo. If an error
+// occurs, *RepoMetadata will be nil, and the error will be non-nil.

	"github.com/go-github-metrics-1/pkg/sdkstats"
	"github.com/learnoperators/go-github-metrics/pkg/sdkstats"

	fmt.Printf("Failed to get stats for query %q %v\n", r, err)
	fmt.Printf("Failed to get stats for queries %v: %v\n", r.Queries, err)

Dev #12

Are you sure you want to change the base?

Dev #12

Uh oh!

Conversation

bharathi-tenneti commented Oct 31, 2019

Uh oh!

joelanford 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!

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!

joelanford commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!