stdbin: adding mkdir #240

i4ki · 2017-09-27T02:50:03Z

This PR starts a long road to address #230.

As I'm using a lot of windows nowadays... I'll start scratching my itches, by adding the programs I'm missing in windows. The work for the stdbin build system and integrated tests could come up in another PR.

Another PR could be created to copy our programs from c0defellas/enzo to here also.

p.s.: the name of the branch is add-stdbin-pwd because pwd was my initial target for this development, but soon realized I was unable to create the stdbin directory in windows without right-click in explorer ... Then mkdir priority escalated very fast =P

Signed-off-by: i4k <[email protected]>

codecov-io · 2017-09-27T02:50:07Z

Codecov Report

Merging #240 into master will decrease coverage by 0.1%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##           master     #240      +/-   ##
==========================================
- Coverage   57.05%   56.95%   -0.11%     
==========================================
  Files          26       27       +1     
  Lines        4403     4416      +13     
==========================================
+ Hits         2512     2515       +3     
- Misses       1651     1660       +9     
- Partials      240      241       +1

Impacted Files	Coverage Δ
stdbin/mkdir/main.go	`23.07% <23.07%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6779f6a...e8d96ad. Read the comment docs.

i4ki · 2017-09-27T02:51:13Z

Makefile

@@ -7,8 +7,9 @@ buildargs = -ldflags "-X main.VersionString=$(version)" -v
 all: build test install

 build:
-	go build $(buildargs) -o ./cmd/nash/nash ./cmd/nash
-	go build $(buildargs) -o ./cmd/nashfmt/nashfmt ./cmd/nashfmt
+	cd cmd/nash && go build $(buildargs) 


this change was needed because on windows the generated binaries must have .exe extensions.

Signed-off-by: i4k <[email protected]>

katcipis · 2017-09-27T11:50:28Z

stdbin/mkdir/main.go

+	}
+	err := mkdirs(os.Args[1:])
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "error: %s\n", err.Error())


No need to call err.Error

katcipis

LGTM, just small crap because I'm annoying as hell =D

katcipis · 2017-09-27T11:51:57Z

stdbin/mkdir/main.go

+
+func mkdirs(dirnames []string) error {
+	for _, d := range dirnames {
+		if err := os.MkdirAll(d, 0644); err != nil {


cant you pass os.ModeDir here instead of 0644 ?

always creating all dirs seems nice, I always do it =).

another thing very common is to ignore if the dir already exists, in the end I always use "-p" for this. What you think about it being the default ?

yes, makes sense. That's what I commonly do also.

cant you pass os.ModeDir here instead of 0644

0644 are the permission bits here, not the file mode. Sorry, did I misunderstand you?

From the name I thought the os.ModeDir would be an integer with the permission bits (indicating the default permissions for directories). My mistake =)

katcipis · 2017-09-27T11:53:23Z

stdbin/mkdir/mkdir_test.go

+		if s, err := os.Stat(testdir); err != nil {
+			t.Fatal(err)
+		} else if s.Mode()&os.ModeDir != os.ModeDir {
+			t.Errorf("Invalid directory mode: %v", s.Mode())


Why Errorf instead of Fatalf since all other checks results on Fatal ?

I'm idiot =)

auheauheauheuaheauheauhe yeah like no one else does this kind of thing =P

katcipis · 2017-09-27T11:54:50Z

stdbin/mkdir/mkdir_test.go

+	defer os.RemoveAll(tmpDir)
+	for _, p := range testDirs {
+		testdir := filepath.FromSlash(path.Join(tmpDir, p))
+		err = mkdirs([]string{testdir})


Isn't a good idea to test with arrays bigger than one too ? Also, what happens if the array is empty ? (can test it on the function or on the final cmd)

yes, there are missing test cases. I made this in a rush... =/
thanks

Thanks for working on this =). We can add an TODO too ;-)

i4ki · 2017-09-29T18:07:10Z

Makefile

@@ -23,6 +24,7 @@ install: build
 	cp -p ./cmd/nashfmt/nashfmt $(NASHROOT)/bin
 	rm -rf $(NASHROOT)/stdlib
 	cp -pr ./stdlib $(NASHROOT)/stdlib
+	cp -pr ./stdbin/mkdir/mkdir $(NASHROOT)/bin/mkdir


this install still only works on unix, I wll address this in the windows branch soon.

thanks, I dont even have an env to test this kind of stuff =P (nor the patience)

Signed-off-by: i4k <[email protected]>

i4ki · 2017-10-01T15:32:15Z

@katcipis Did you take a look at changes made?
I need this code in the fix-windows branch to finish the tests there :)

vitorarins

LGTM

starting stdbin with mkdir

a632c9d

Signed-off-by: i4k <[email protected]>

i4ki commented Sep 27, 2017

View reviewed changes

remove debug print

8b07b57

Signed-off-by: i4k <[email protected]>

i4ki requested review from katcipis, vitorarins and m0nad September 27, 2017 02:52

katcipis reviewed Sep 27, 2017

View reviewed changes

katcipis requested changes Sep 27, 2017

View reviewed changes

i4ki commented Sep 29, 2017

View reviewed changes

i4ki added 2 commits September 29, 2017 15:22

improve tests

7a770bd

Signed-off-by: i4k <[email protected]>

test dir already exists

80d066c

Signed-off-by: i4k <[email protected]>

Merge branch 'master' into add-stdbin-pwd

e8d96ad

i4ki mentioned this pull request Oct 1, 2017

Add pwd #244

Merged

vitorarins approved these changes Oct 6, 2017

View reviewed changes

i4ki merged commit 1c51e3d into master Oct 7, 2017

i4ki deleted the add-stdbin-pwd branch October 7, 2017 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdbin: adding mkdir #240

stdbin: adding mkdir #240

i4ki commented Sep 27, 2017

codecov-io commented Sep 27, 2017 •

edited

Loading

i4ki Sep 27, 2017

katcipis Sep 27, 2017

katcipis left a comment

katcipis Sep 27, 2017

katcipis Sep 27, 2017

i4ki Sep 27, 2017

i4ki Sep 27, 2017 •

edited

Loading

katcipis Sep 27, 2017

i4ki Sep 29, 2017

katcipis Sep 27, 2017

i4ki Sep 27, 2017

katcipis Sep 27, 2017

katcipis Sep 27, 2017

i4ki Sep 27, 2017

katcipis Sep 27, 2017

i4ki Sep 29, 2017

katcipis Sep 29, 2017

i4ki commented Oct 1, 2017 •

edited

Loading

vitorarins left a comment

stdbin: adding mkdir #240

stdbin: adding mkdir #240

Conversation

i4ki commented Sep 27, 2017

codecov-io commented Sep 27, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katcipis left a 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

i4ki Sep 27, 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

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

i4ki commented Oct 1, 2017 • edited Loading

vitorarins left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 27, 2017 •

edited

Loading

i4ki Sep 27, 2017 •

edited

Loading

i4ki commented Oct 1, 2017 •

edited

Loading