refactor requested changes, minor improvements

- `project`
  - `checkCfgFilenames`
    - Improve function and code comments
    - Use boolean value indicating whether value was found in actual filenames.
      If manifest file was not found, return `errProjectNotFound`. Use boolean
      to determine if lock file was not found instead of length check.
  - `TestCheckCfgFilenames` - Add code comments for test cases explaining the
    expected behavior
- `fs`
  - `ReadActualFilenames`
    - Use cleaner check(`<=` ➡ `==`) to see if `names` parameter for
      `ReadActualFilenames` actually has any values.
    - Use `Readdirnames` instead of `Readdir`. This is expected to perform
      better in most cases.
  - `TestReadActualFilenames` - Add code comments for test cases explaining the
    expected behavior
- general
  - Make line length for code comments consistent(90), add periods where
    appropriate.
  - String formatting, use %q instead of '%s'

Author: sudo-suhas

context_test.go

@@ -269,9 +269,8 @@ func TestLoadProjectCfgFileCase(t *testing.T) {
 		t.Skip("skip this test on non-Windows, non-macOS")
 	}
 
-	// Here we test that a manifest filename with incorrect case
-	// throws an error. Similar error will also be thrown for the
-	// lock file as well which has been tested in
+	// Here we test that a manifest filename with incorrect case throws an error. Similar
+	// error will also be thrown for the lock file as well which has been tested in
 	// `project_test.go#TestCheckCfgFilenames`. So not repeating here.
 
 	h := test.NewHelper(t)
@@ -299,7 +298,7 @@ func TestLoadProjectCfgFileCase(t *testing.T) {
 	}
 
 	expectedErrMsg := fmt.Sprintf(
-		"manifest filename '%s' does not match '%s'",
+		"manifest filename %q does not match %q",
 		invalidMfName, ManifestName,
 	)
 

internal/fs/fs.go

@@ -271,12 +271,13 @@ var errPathNotDir = errors.New("given path is not a directory")
 // On case sensitive file systems like ext4, it will check if those files exist using
 // `os.Stat` and return a map with key and value as filenames which exist in the folder.
 //
-// Otherwise, it reads the contents of the directory
+// Otherwise, it reads the contents of the directory and returns a map which has the
+// given file name as the key and actual filename as the value if it was found.
 func ReadActualFilenames(dirPath string, names []string) (map[string]string, error) {
 	actualFilenames := make(map[string]string, len(names))
-	if len(names) <= 0 {
-		// This isn't expected to happen for current usage.
-		// Adding edge case handling, maybe useful in future
+	if len(names) == 0 {
+		// This isn't expected to happen for current usage. Adding edge case handling,
+		// as it may be useful in future.
 		return actualFilenames, nil
 	}
 	// First, check that the given path is valid and it is a directory
@@ -289,23 +290,23 @@ func ReadActualFilenames(dirPath string, names []string) (map[string]string, err
 		return nil, errPathNotDir
 	}
 
-	// Ideally, we would use `os.Stat` for getting the actual file names
-	// but that returns the name we passed in as an argument and not the actual filename.
-	// So we are forced to list the directory contents and check
-	// against that. Since this check is costly, we do it only if absolutely necessary.
+	// Ideally, we would use `os.Stat` for getting the actual file names but that returns
+	// the name we passed in as an argument and not the actual filename. So we are forced
+	// to list the directory contents and check against that. Since this check is costly,
+	// we do it only if absolutely necessary.
 	caseSensitive, err := IsCaseSensitiveFilesystem(dirPath)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to read actual filenames")
 	}
 	if caseSensitive {
-		// There will be no difference between actual filename and given filename
-		// So just check if those files exist.
+		// There will be no difference between actual filename and given filename. So
+		// just check if those files exist.
 		for _, name := range names {
 			_, err := os.Stat(filepath.Join(dirPath, name))
 			if err == nil {
 				actualFilenames[name] = name
 			} else if !os.IsNotExist(err) {
-				// Some unexpected err, return it.
+				// Some unexpected err, wrap and return it.
 				return nil, errors.Wrap(err, "failed to read actual filenames")
 			}
 		}
@@ -318,29 +319,27 @@ func ReadActualFilenames(dirPath string, names []string) (map[string]string, err
 	}
 	defer dir.Close()
 
-	// Pass -1 to read all files in directory
-	files, err := dir.Readdir(-1)
+	// Pass -1 to read all filenames in directory
+	filenames, err := dir.Readdirnames(-1)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to read actual filenames")
 	}
 
-	// namesMap holds the mapping from lowercase name to search name.
-	// Using this, we can avoid repeatedly looping through names.
+	// namesMap holds the mapping from lowercase name to search name. Using this, we can
+	// avoid repeatedly looping through names.
 	namesMap := make(map[string]string, len(names))
 	for _, name := range names {
 		namesMap[strings.ToLower(name)] = name
 	}
 
-	for _, file := range files {
-		if file.Mode().IsRegular() {
-			searchName, ok := namesMap[strings.ToLower(file.Name())]
-			if ok {
-				// We are interested in this file, case insensitive match successful
-				actualFilenames[searchName] = file.Name()
-				if len(actualFilenames) == len(names) {
-					// We found all that we were looking for
-					return actualFilenames, nil
-				}
+	for _, filename := range filenames {
+		searchName, ok := namesMap[strings.ToLower(filename)]
+		if ok {
+			// We are interested in this file, case insensitive match successful.
+			actualFilenames[searchName] = filename
+			if len(actualFilenames) == len(names) {
+				// We found all that we were looking for.
+				return actualFilenames, nil
 			}
 		}
 	}

internal/fs/fs_test.go

@@ -275,10 +275,12 @@ func TestReadActualFilenames(t *testing.T) {
 	h.TempDir("")
 	tmpPath := h.Path(".")
 
+	// First, check the scenarios for which we expect an error.
 	_, err := ReadActualFilenames(filepath.Join(tmpPath, "does_not_exists"), []string{""})
 	switch {
 	case err == nil:
 		t.Fatal("expected err for non-existing folder")
+	// use `errors.Cause` because the error is wrapped and returned
 	case !os.IsNotExist(errors.Cause(err)):
 		t.Fatalf("unexpected error: %+v", err)
 	}
@@ -296,11 +298,23 @@ func TestReadActualFilenames(t *testing.T) {
 		names       []string
 		want        map[string]string
 	}{
-		{nil, nil, map[string]string{}}, {
+		// If we supply no filenames to the function, it should return an empty map.
+		{nil, nil, map[string]string{}},
+		// If the directory contains the given file with different case, it should return
+		// a map which has the given filename as the key and actual filename as the value.
+		{
 			[]string{"test1.txt"},
 			[]string{"Test1.txt"},
 			map[string]string{"Test1.txt": "test1.txt"},
-		}, {
+		},
+		// 1. If the given filename is same as the actual filename, map should have the
+		//    same key and value for the file.
+		// 2. If the given filename is present with different case for file extension,
+		//    it should return a map which has the given filename as the key and actual
+		//    filename as the value.
+		// 3. If the given filename is not present even with a different case, the map
+		//    returned should not have an entry for that filename.
+		{
 			[]string{"test2.txt", "test3.TXT"},
 			[]string{"test2.txt", "Test3.txt", "Test4.txt"},
 			map[string]string{

project.go

@@ -43,10 +43,16 @@ func findProjectRoot(from string) (string, error) {
 }
 
 // checkCfgFilenames validates filename case for the manifest and lock files.
-// This is relevant on case-insensitive systems like Windows.
+//
+// This is relevant on case-insensitive file systems like Windows and macOS.
+//
+// If manifest file is not found, it returns an error indicating the project could not be
+// found. If it is found but the case does not match, an error is returned. If a lock
+// file is not found, no error is returned as lock file is optional. If it is found but
+// the case does not match, an error is returned.
 func checkCfgFilenames(projectRoot string) error {
-	// ReadActualFilenames is actually costly.  Since this check is not relevant
-	// to case-sensitive filesystems like ext4, try for an early return.
+	// ReadActualFilenames is actually costly. Since this check is not relevant to
+	// case-sensitive filesystems like ext4(linux), try for an early return.
 	caseSensitive, err := fs.IsCaseSensitiveFilesystem(projectRoot)
 	if err != nil {
 		return errors.Wrap(err, "could not check validity of configuration filenames")
@@ -61,20 +67,22 @@ func checkCfgFilenames(projectRoot string) error {
 		return errors.Wrap(err, "could not check validity of configuration filenames")
 	}
 
-	// Since this check is done after `findProjectRoot`, we can assume that
-	// manifest file will be present. Even if it is not, error will still be thrown.
-	// But error message will be a tad inaccurate.
-	actualMfName := actualFilenames[ManifestName]
+	actualMfName, found := actualFilenames[ManifestName]
+	if !found {
+		// Ideally this part of the code won't ever be executed if it is called after
+		// `findProjectRoot`. But be thorough and handle it anyway.
+		return errProjectNotFound
+	}
 	if actualMfName != ManifestName {
-		return fmt.Errorf("manifest filename '%s' does not match '%s'", actualMfName, ManifestName)
+		return fmt.Errorf("manifest filename %q does not match %q", actualMfName, ManifestName)
 	}
 
-	// If a file is not found, the string map returned by `fs.ReadActualFilenames`
-	// will not have an entry for the given filename. Since the lock file
-	// is optional, we should check for equality only if it was found.
-	actualLfName := actualFilenames[LockName]
-	if len(actualLfName) > 0 && actualLfName != LockName {
-		return fmt.Errorf("lock filename '%s' does not match '%s'", actualLfName, LockName)
+	// If a file is not found, the string map returned by `fs.ReadActualFilenames` will
+	// not have an entry for the given filename. Since the lock file is optional, we
+	// should check for equality only if it was found.
+	actualLfName, found := actualFilenames[LockName]
+	if found && actualLfName != LockName {
+		return fmt.Errorf("lock filename %q does not match %q", actualLfName, LockName)
 	}
 
 	return nil

project_test.go

@@ -70,7 +70,7 @@ func TestCheckCfgFilenames(t *testing.T) {
 
 	errMsgFor := func(filetype, filename string) func(string) string {
 		return func(name string) string {
-			return fmt.Sprintf("%s filename '%s' does not match '%s'", filetype, name, filename)
+			return fmt.Sprintf("%s filename %q does not match %q", filetype, name, filename)
 		}
 	}
 
@@ -85,29 +85,44 @@ func TestCheckCfgFilenames(t *testing.T) {
 		createFiles []string
 		wantErrMsg  string
 	}{
+		// No error should be returned when the project contains a valid manifest file
+		// but no lock file.
 		{false, []string{ManifestName}, ""},
+		// No error should be returned when the project contains a valid manifest file as
+		// well as a valid lock file.
 		{false, []string{ManifestName, LockName}, ""},
-		{true, nil, manifestErrMsg("")},
+		// Error indicating the project was not found should be returned if a manifest
+		// file is not found.
+		{true, nil, errProjectNotFound.Error()},
+		// Error should be returned if the project has a manifest file with invalid name
+		// but no lock file.
 		{true, []string{invalidMfName}, manifestErrMsg(invalidMfName)},
+		// Error should be returned if the project has a valid manifest file and an
+		// invalid lock file.
 		{true, []string{ManifestName, invalidLfName}, lockErrMsg(invalidLfName)},
 	}
 
 	for _, c := range cases {
 		h := test.NewHelper(t)
 		defer h.Cleanup()
 
+		// Create a temporary directory which we will use as the project folder.
 		h.TempDir("")
 		tmpPath := h.Path(".")
 
+		// Create any files that are needed for the test before invoking
+		// `checkCfgFilenames`.
 		for _, file := range c.createFiles {
 			h.TempFile(file, "")
 		}
 		err := checkCfgFilenames(tmpPath)
 
 		if c.wantErr {
 			if err == nil {
+				// We were expecting an error but did not get one.
 				t.Fatalf("unexpected error message: \n\t(GOT) nil\n\t(WNT) %s", c.wantErrMsg)
 			} else if err.Error() != c.wantErrMsg {
+				// We got an error but it is not the one we were expecting.
 				t.Fatalf("unexpected error message: \n\t(GOT) %s\n\t(WNT) %s", err.Error(), c.wantErrMsg)
 			}
 		} else if err != nil {