review 2024-10-04

Makefile

@@ -4,7 +4,6 @@
 # Allow environment variable override for running targets within Dockerfile
 VERSION ?= $(shell git describe --always --dirty --tags | egrep -o '([0-9]+\.){1,2}[0-9]?[a-zA-Z0-9-]+')
 BUILD := $(shell date +%FT%T%z)
-# [x] fetching git tag here
 LASTTAG := $(shell git fetch --tags && git describe --abbrev=0 --tags)
 BUILD_FLAGS :=
 
@@ -14,6 +13,7 @@ BINARY := password-${VERSION}
 REQUIRED_COVERAGE ?= 80
 
 test:
+# FIXME: the "race" flag could be omitted since it's a tool running from CLI
 	@go test -v -race ./...
 
 install:

Readme.md

@@ -11,11 +11,6 @@ It was made with love using <https://github.com/spf13/cobra-cli>
 
 ## How to install
 
-<!-- FIXME: why do you care about Git tags? -->
-<!-- [x] This tag is needed by Makefile to generate binary, let me remove this and just let makefile fetch the tags-->
-<!-- ```shell
-git fetch --all --tags
-``` -->
 There are two ways to install the `password` utility
 
 ### Method 1: Download a pre-built binary
@@ -82,8 +77,6 @@ The purpose of each option is as follows:
 
 If you have followed the [Installation instructions](#how-to-install), you can use the following examples. You can check [All available options](#available-options-for-controlling-the-generated-password).
 
-<!-- [x] Done adding examples -->
-
 - To generate a password with default options
 
     ```shell
@@ -118,18 +111,16 @@ For safety, the code will not remove an exising file, matching the default passw
 make test
 ```
 
-<!-- ### How to test it without installing -->
-<!-- FIXME: this is outdated. -->
-<!-- [x] removing this section-->
-<!-- Simply replace `~/go/bin/password` with `go run main.go` and you can try everything described above. -->
+### How to test it without installing
+<!-- FIXME: I believe this section should be left because someone may want to run it directly in the source code. -->
+<!-- FIXME: you could also include instructions on how to build the command from the source code. -->
+Simply replace `~/go/bin/password` with `go run main.go` and you can try everything described above.
 
 ## New Requirements
 
 The next you're asked is to create a hierarchy of commands in this way:
 
-<!-- [x] Done -->
 - `password` is the master command
-  <!-- [x] Done -->
   - `generate` is the actual command to generate the password
   <!-- [x] Not started yet -->
   - `validate` checks the password against a predefined set of rules and shares those with the user.

cmd/password.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"fmt"
+	// FIXME: use the log/slog pkg which should be the new standard.
 	"log"
 	"os"
 
@@ -65,7 +66,9 @@ func init() {
 		password.DefaultFilePath,
 		fmt.Sprintf("filepath (when %d is provided for %s)", password.ToFile, FlagNameOutput),
 	)
+	// BUG: use structured logging with Fields and so on
 	logOpts := log.LstdFlags | log.Lshortfile | log.Ldate | log.Ltime | log.LUTC
+	// BUG: `Length:0x7, Count:0x5,` printed on the shell is not user-friendly.
 	logger = log.New(os.Stderr, "password generator: ", logOpts)
 }
 
@@ -89,6 +92,8 @@ func runPasswordGenerator(cmd *cobra.Command, args []string) error {
 	}
 	logMesg := `generating password(s) with the following options %#v`
 	logger.Printf(logMesg, passOptions)
+	// FIXME: here, it makes sense to refactor the code by taking out the "count" from the Options struct.
+	// The Options struct describes the characteristics of the password. The "count" is something else. The Generate function should be invoked as many times as we want from the count and every time must return a single password (to make it more reusable).
 	passwords, err := password.Generate(&passOptions)
 	if err != nil {
 		return err

go.mod

@@ -2,13 +2,15 @@ module amritsingh183/password
 
 go 1.22.0
 
-require github.com/spf13/cobra v1.8.1
+require (
+	github.com/spf13/cobra v1.8.1
+	github.com/stretchr/testify v1.9.0
+)
 
 require (
 	github.com/davecgh/go-spew v1.1.1 // indirect
 	github.com/inconshreveable/mousetrap v1.1.0 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/spf13/pflag v1.0.5 // indirect
-	github.com/stretchr/testify v1.9.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )

go.sum

@@ -12,6 +12,7 @@ github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg=
 github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

internal/password/password.go

@@ -1,11 +1,12 @@
 package password
 
 import (
-	"amritsingh183/password/internal/util"
 	"errors"
 	"io"
 	"os"
 	"unsafe"
+
+	"amritsingh183/password/internal/util"
 )
 
 // // FIXME: let's get rid of this struct. Let's group the parameters of the password generator in another struct with a different name. For the target, you can leverage the interface io.Writer that can be accepted as a parameter of the function that has to write the password somewhere.
@@ -40,6 +41,8 @@ func Generate(o *Options) ([][]byte, error) {
 
 // Write writes the password to destination.
 // It closes the file when destination is a file
+// FIXME: the signature might be: func Write(w io.writer, passwords []string) error {}
+// where io.Writer might be: StdOut, a file handle, a mock, etc.
 func Write(data [][]byte, o *Options) error {
 	var w io.Writer
 	var err error
@@ -56,6 +59,7 @@ func Write(data [][]byte, o *Options) error {
 	}
 	var stringPassword string
 	for _, bytePassword := range data {
+		// [Q]: can you explain me why are you using so often the "unsafe" pkg? It should be used seldomly. Maybe I'm not getting your thoughts.
 		usPtr := unsafe.Pointer(&bytePassword)
 		strPtr := (*string)(usPtr)
 		stringPassword = *strPtr

internal/password/password_test.go

@@ -1,7 +1,6 @@
 package password
 
 import (
-	"amritsingh183/password/internal/util"
 	"bytes"
 	"fmt"
 	"io"
@@ -10,6 +9,8 @@ import (
 	"testing"
 	"unsafe"
 
+	"amritsingh183/password/internal/util"
+
 	"github.com/stretchr/testify/assert"
 )
 
@@ -171,5 +172,55 @@ func TestWrite(t *testing.T) {
 		msg := fmt.Sprintf("expected password to be of length %d", passwordLength)
 		assert.Equal(t, passwordLength, len(passwd), msg)
 	})
+}
+
+// tapStdOut provides mechanism to tap into the stdout
+// useful for testing only
+// FIXME:you can easily take advantage of the io.Writer interface and save the values in memory.
+// https://quii.gitbook.io/learn-go-with-tests/go-fundamentals/dependency-injection#write-enough-code-to-make-it-pass
+type tapStdOut struct {
+	outChan chan string
+	errChan chan error
+
+	writeTo      *os.File
+	stdOutbackup *os.File
+}
+
+// Start starts the tapping process and backsup stdout
+func (tapper *tapStdOut) Start() error {
+	tapper.stdOutbackup = os.Stdout
+	rf, wf, err := os.Pipe()
+	if err != nil {
+		return err
+	}
+	os.Stdout = wf
+	tapper.writeTo = wf
+	tapper.outChan = make(chan string)
+	tapper.errChan = make(chan error)
+	go tapper.read(rf)
+	return nil
+}
+
+// read reads from readpipe into channel of tapper
+// must be called in a go routine to prevent
+// blocking writes to writepipe (such as stdout)
+func (tapper *tapStdOut) read(readFrom *os.File) {
+	var output bytes.Buffer
+	_, err := io.Copy(&output, readFrom)
+	tapper.errChan <- err
+	tapper.outChan <- output.String()
+}
 
+// Flush Stops the tapping process, sends the stored output and restores stdout
+func (tapper *tapStdOut) Flush() (string, error) {
+	err := tapper.writeTo.Close()
+	if err != nil {
+		return "", err
+	}
+	err = <-tapper.errChan
+	if err != nil {
+		return "", err
+	}
+	os.Stdout = tapper.stdOutbackup
+	return <-tapper.outChan, nil
 }

internal/util/data.go

@@ -60,6 +60,9 @@ func GenerateKey(n int, includeSpecials bool) ([]byte, error) {
 }
 
 // isValid validates the length of key to be generated
+// TODO: this could also be replaced by some validation pkg.
+// BUG: I'd like to generate unsecure keys. You should allow the users to do so.
+// Maybe, you can print a warning that the password is not secure enough and the reason.
 func IsValidKeyLength(n int) error {
 	if n > MaxKeyLength {
 		return fmt.Errorf("maximum key length is %d but %d was provided", MaxKeyLength, n)
@@ -69,6 +72,9 @@ func IsValidKeyLength(n int) error {
 	}
 	return nil
 }
+
+// TODO: this is really needed? Or we can solve it with an easier way?
+// I mean the key and all of the other stuff. The code will be much more simplified.
 func generate(n int, letterBytes string) ([]byte, error) {
 	randBytes := make([]byte, 10240)
 	_, err := io.ReadFull(cryptoRand.Reader, randBytes)