Fix docs and add a validator tool for our api docs #850

harshavardhana · 2017-10-19T09:29:44Z

No description provided.

nitisht · 2017-10-20T09:00:42Z

docs/validator.go

+			exec.Command("gofmt", cmdArgs...).Run()
+
+			cmdArgs = []string{"-w", testFilePath}
+			exec.Command("goimports", cmdArgs...).Run()


validation fails for FGetEncryptedObject, looks like encrypt is not getting imported

Failed to validate code at docs/API.md:18247 // Generate a master symmetric key key := encrypt.NewSymmetricKey([]byte("my-secret-key-00")) // Build the CBC encryption material cbcMaterials, err := encrypt.NewCBCSecureMaterials(key) if err != nil { fmt.Println(err) return } err = minioClient.FGetEncryptedObject("mybucket", "myobject", "/tmp/myobject", cbcMaterials) if err != nil { fmt.Println(err) return }

It runs fine on CI looks like you have a local issue, goimports has a bug btw it stops scanning imports if it reaches a directory which it cannot access.

Go to the directory and check manually what is going on.

goimports has a bug btw it stops scanning imports if it reaches a directory which it cannot access.

This is what I faced.

Go to the directory and check manually what is going on.

will do

nitisht · 2017-10-20T09:03:58Z

docs/validator.go

+				fmt.Printf("Failed to validate code at %s:%d\n%s", docPath, p.Pos, p.Text)
+				os.Exit(255)
+			}
+			fmt.Printf("Successfully validated %s code at %s:%d\n", p.Lang, docPath, p.Pos)


p.Pos is a little confusing, not clear what position it is referring to

Successfully validated go code at docs/API.md:7695 Successfully validated go code at docs/API.md:8261 Successfully validated go code at docs/API.md:8847 Successfully validated go code at docs/API.md:9279 Successfully validated go code at docs/API.md:10467 Successfully validated go code at docs/API.md:11698 Successfully validated go code at docs/API.md:13270 Successfully validated go code at docs/API.md:14553 Successfully validated go code at docs/API.md:15424 Successfully validated go code at docs/API.md:16389 Successfully validated go code at docs/API.md:17463

A position is the position in file with number of characters. It doesn't equate to line number.

in that case, I dont feel it adds any value, can we remove it?

balamurugana · 2017-10-21T00:02:51Z

docs/validator.go

+)
+
+var templateMain = `
+// +build ignore


This text can be in separate file like checker.go.template

nitisht

LGTM and Tested

harshavardhana · 2017-10-25T08:22:36Z

@balamurugana can you review this again?

balamurugana · 2017-10-25T14:30:41Z

docs/checker.go.tpl

@@ -0,0 +1,23 @@
+// +build ignore


I think this line can be removed.

tpl extension is too limited. IMO we would have fully expanded .template

We cannot remove build ignore, it is needed to avoid conflicting with other packages in current directory .tpl is acceptable name most projects like hugo etc support this. We don't need to call it .template

balamurugana · 2017-10-25T14:32:35Z

docs/validator.go

@@ -0,0 +1,167 @@
+// +build ignore


I think we should build on make to ensure its compiling all the time.

There is no make in minio-go, We ensure by running on travis.

Added a make in minio-go

balamurugana · 2017-10-25T14:40:13Z

docs/validator.go

+		Name:  "skip",
+		Value: 2,
+		Usage: "Skip entries before validating the code.",
+	},


I think we would need to have a template like mc --help to show how this can be used with examples.

Will add that.

balamurugana · 2017-10-25T14:45:47Z

docs/validator.go

+			entryN++
+
+			cmdArgs := []string{"-s", "-w", "-l", testFilePath}
+			exec.Command("gofmt", cmdArgs...).Run()


You could have a function or closure hide this logic here. This way you could have cmdArgs globally. If available I prefer long option than short option for readability (if available)

function runGofmt(string testFilePath) { ... }

balamurugana · 2017-10-25T14:47:46Z

docs/validator.go

+			exec.Command("gofmt", cmdArgs...).Run()
+
+			cmdArgs = []string{"-w", testFilePath}
+			exec.Command("goimports", cmdArgs...).Run()


Same as above.

balamurugana · 2017-10-25T14:48:35Z

docs/validator.go

+
+			// Go build current directory
+			cmdArgs = []string{"build", "-o", testBinPath, testFilePath}
+			if cerr := exec.Command("go", cmdArgs...).Run(); cerr != nil {


same as above.

I think we don't need have args -o testBinPath. This intention here is to check whether testFilePath compiles are not.

Also we would need to show compilation error if any. This way we know what exact problem in the code.

-o builds a binary with right name. Compilation error is displayed with the cerr.

harshavardhana · 2017-10-26T18:09:17Z

Can you review this again Bala?

harshavardhana · 2017-10-27T02:22:25Z

PR is updated @balamurugana please check.

balamurugana

LGTM

balamurugana · 2017-10-27T03:57:41Z

Makefile

@@ -0,0 +1,17 @@
+all: checks


nitisht requested review from balamurugana and nitisht October 19, 2017 09:31

harshavardhana force-pushed the fix-docs branch 2 times, most recently from b954d80 to a10d11e Compare October 19, 2017 09:32

harshavardhana changed the title ~~Fix docs and add a validator function for each of our docs.~~ Fix docs and add a validator tool for our api docs Oct 19, 2017

harshavardhana force-pushed the fix-docs branch from a10d11e to 88b5184 Compare October 19, 2017 09:45

nitisht reviewed Oct 20, 2017

View reviewed changes

harshavardhana force-pushed the fix-docs branch from 88b5184 to fad67d4 Compare October 20, 2017 09:37

balamurugana requested changes Oct 21, 2017

View reviewed changes

harshavardhana force-pushed the fix-docs branch 2 times, most recently from 67d3a06 to 4c98239 Compare October 21, 2017 00:57

nitisht approved these changes Oct 21, 2017

View reviewed changes

balamurugana requested changes Oct 25, 2017

View reviewed changes

harshavardhana force-pushed the fix-docs branch 5 times, most recently from f4ae19d to 9a79ce3 Compare October 26, 2017 07:16

harshavardhana force-pushed the fix-docs branch 2 times, most recently from a152b6b to 882ec87 Compare October 27, 2017 02:20

Fix docs and add a validator tool for our api docs

340f70f

harshavardhana force-pushed the fix-docs branch from 882ec87 to 340f70f Compare October 27, 2017 02:42

balamurugana approved these changes Oct 27, 2017

View reviewed changes

Makefile

@@ -0,0 +1,17 @@

all: checks

Copy link

Member

balamurugana Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

nitisht merged commit 0739f1d into minio:master Oct 27, 2017

harshavardhana deleted the fix-docs branch October 27, 2017 05:40

harshavardhana mentioned this pull request Nov 1, 2017

API doc is missing possible values for PutObject.options #841

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix docs and add a validator tool for our api docs #850

Fix docs and add a validator tool for our api docs #850

harshavardhana commented Oct 19, 2017

nitisht Oct 20, 2017

harshavardhana Oct 20, 2017 •

edited

Loading

nitisht Oct 20, 2017

nitisht Oct 20, 2017

harshavardhana Oct 20, 2017

nitisht Oct 20, 2017

balamurugana Oct 21, 2017

harshavardhana Oct 21, 2017

nitisht left a comment

harshavardhana commented Oct 25, 2017

balamurugana Oct 25, 2017

harshavardhana Oct 25, 2017

balamurugana Oct 25, 2017

harshavardhana Oct 25, 2017

harshavardhana Oct 26, 2017

balamurugana Oct 25, 2017

harshavardhana Oct 25, 2017

balamurugana Oct 25, 2017

balamurugana Oct 25, 2017

balamurugana Oct 25, 2017

harshavardhana Oct 25, 2017

harshavardhana commented Oct 26, 2017

harshavardhana commented Oct 27, 2017

balamurugana left a comment

balamurugana Oct 27, 2017

Fix docs and add a validator tool for our api docs #850

Fix docs and add a validator tool for our api docs #850

Conversation

harshavardhana commented Oct 19, 2017

Choose a reason for hiding this comment

harshavardhana Oct 20, 2017 • edited Loading

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

nitisht left a comment

Choose a reason for hiding this comment

harshavardhana commented Oct 25, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana commented Oct 26, 2017

harshavardhana commented Oct 27, 2017

balamurugana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harshavardhana Oct 20, 2017 •

edited

Loading