Cleaning up the code mostly for issue #53

Changes:
- Removed InstallDependency function. Now it's the Install method of the Profile struct.
- Renamed Filter.GetName method to GetFriendlyName and removed the 'Running filter' message from it
- Cleaning up InstallDependency function spawned a few new methods (they should probably be moved somewhere else in the code). The methods: Filter.GetDownloadUrl, Filter.GetIdName, Filter.Download (all of the new functions/methods have are documented with comments).

This commit doesn't change how the code works (with some small exceptions). Next cleanup step will probably slightly change the logic of the code.

regolith/filters.go

@@ -30,12 +30,12 @@ func RegisterFilters() {
 // absoluteLocation is an absolute path to the root folder of the filter.
 // In case of local filters it's a root path of the project.
 func (filter *Filter) RunFilter(absoluteLocation string) error {
-	Logger.Info(filter.GetName())
+	Logger.Infof("Running filter %s", filter.GetFriendlyName())
 	start := time.Now()
 
 	// Disabled filters are skipped
 	if filter.Disabled == true {
-		Logger.Infof("Filter '%s' is disabled, skipping.", filter.GetName())
+		Logger.Infof("Filter '%s' is disabled, skipping.", filter.GetFriendlyName())
 		return nil
 	}
 

regolith/profiles.go

@@ -98,7 +98,7 @@ func RunProfile(profileName string) error {
 		path, _ := filepath.Abs(".")
 		err := filter.RunFilter(path)
 		if err != nil {
-			return wrapError(fmt.Sprintf("%s failed", filter.GetName()), err)
+			return wrapError(fmt.Sprintf("%s failed", filter.GetFriendlyName()), err)
 		}
 	}
 

regolith/project.go

@@ -61,12 +61,13 @@ type ExportTarget struct {
 	// Path          string `json:"path"`
 }
 
-// TODO This function is kinda weird. Probably should not include "Running filter"
-func (filter Filter) GetName() string {
+// GetFriendlyName returns the friendly name of the filter that can be used for
+// logging.
+func (filter Filter) GetFriendlyName() string {
 	if filter.Name != "" {
 		return filter.Name
 	}
-	return fmt.Sprintf("Running filter %s", filter.Filter)
+	return filter.Filter
 }
 
 func IsProjectConfigured() bool {

regolith/remote_filters.go

@@ -6,6 +6,7 @@ import (
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
 	"strings"
 
 	getter "github.com/hashicorp/go-getter"
@@ -38,11 +39,9 @@ func IsRemoteFilterCached(url string) bool {
 	return err == nil
 }
 
-/*
-Recursively install dependencies for the entire config.
- - Force mode will overwrite existing dependencies.
- - Non-force mode will only install dependencies that are not already installed.
-*/
+// Recursively install dependencies for the entire config.
+//  - Force mode will overwrite existing dependencies.
+//  - Non-force mode will only install dependencies that are not already installed.
 func InstallDependencies(isForced bool) error {
 	Logger.Infof("Installing dependencies...")
 
@@ -63,7 +62,7 @@ func InstallDependencies(isForced bool) error {
 	}
 
 	for _, profile := range project.Profiles {
-		err := InstallDependency(profile, isForced)
+		err := profile.Install(isForced)
 		if err != nil {
 			return wrapError("Could not install dependency", err)
 		}
@@ -73,133 +72,193 @@ func InstallDependencies(isForced bool) error {
 	return nil
 }
 
-// InstallDependency recursively downloads the filters of a profile and the
-// filters specified in other filters.
-func InstallDependency(profile Profile, isForced bool) error {
-	for _, filter := range profile.Filters {
-		// Get the url of the dependency, which may be constructed
-		var url string
-		if filter.Url != "" {
-			url = FilterNameToUrl(filter.Url, filter.Filter)
-		} else if filter.Filter != "" {
-			url = FilterNameToUrl(StandardLibraryUrl, filter.Filter)
-		} else { // Leaf of profile tree (nothing to install)
-			continue
-		}
-
-		// TODO This needs to be re-implemented
-		// err := ValidateUrl(url)
-		// if err != nil {
-		// 	return err
-		// }
-
-		// Download the filter into the cache folder
-		downloadPath := UrlToPath(url)
-
-		// If downloadPath already exists, we don't need to download it again.
-		// Force mode allows overwriting.
-		if _, err := os.Stat(downloadPath); err == nil {
-			if !isForced {
-				Logger.Warnf("Dependency %s already installed, skipping. Run with '-f' to force.", url)
-				continue
-			} else {
-				Logger.Warnf("Dependency %s already installed, but forcing installation.", url)
-				err := os.RemoveAll(downloadPath)
-				if err != nil {
-					return wrapError("Could not remove installed filter", err)
-				}
-			}
-		}
-
-		Logger.Infof("Installing dependency %s...", url)
+// LoadFilterJsonProfile loads a profile from path to filter.json file of
+// a remote filter and propagates the properties of the parent filter (the
+// filter in config.json or other remote filter that caused creation of this
+// profile).
+func LoadFilterJsonProfile(filterJsonPath string, parentFilter Filter) (*Profile, error) {
+	// Open file
+	file, err := ioutil.ReadFile(filterJsonPath)
+	if err != nil {
+		return nil, wrapError(fmt.Sprintf(
+			"Couldn't find %s", filterJsonPath), err)
+	}
+	// Load data into Profile struct
+	var remoteProfile Profile
+	err = json.Unmarshal(file, &remoteProfile)
+	if err != nil {
+		return nil, wrapError(fmt.Sprintf(
+			"Couldn't load %s: ", filterJsonPath), err)
+	}
+	// Propagate venvSlot property
+	for subfilter := range remoteProfile.Filters {
+		remoteProfile.Filters[subfilter].VenvSlot = parentFilter.VenvSlot
+	}
+	return &remoteProfile, nil
+}
 
-		// Download the filter fresh
-		ok, err := DownloadGitHubUrl(url, downloadPath)
+// Install installs all of the dependencies of the profile
+func (p *Profile) Install(isForced bool) error {
+	for filter := range p.Filters {
+		filter := &p.Filters[filter] // Using pointer is faster than creating copies in the loop and gives more options
+		downloadPath, err := filter.Download(isForced)
+		// TODO - we could use type switch to handle different kinds of errors
+		// here. Download can fail on downloading or on cleaning the download
+		// path. It can also fail when isForced is false and the path already
+		// exists.
 		if err != nil {
-			Logger.Debug(err)
-		}
-		if !ok {
-			Logger.Debug("Failed to download filter " + filter.Filter + " without git")
-			err := getter.Get(downloadPath, url)
-			if err != nil {
-				return err
-			}
-		}
-
-		// Remove 'test' folder, which may be installed via git-getter library
-		// This is a workaround for cases where our own getter library is not
-		// able to download the filter.
-		testFolder := path.Join(downloadPath, "test")
-		if _, err := os.Stat(testFolder); err == nil {
-			os.RemoveAll(testFolder)
-			if err != nil {
-				Logger.Debug("Could not remove test folder", err)
-			}
+			return wrapError("Could not download filter", err) // TODO - I don't think this should break the entire install
+		} else if downloadPath == "" {
+			continue // Not a remote filter, skip
 		}
 
 		// Move filters 'data' folder contents into 'data'
-		filterName := strings.Split(path.Clean(url), "/")[3]
-		localDataPath := path.Join(profile.DataPath, filterName)
+		filterName := filter.GetIdName()
+		localDataPath := path.Join(p.DataPath, filterName)
 		remoteDataPath := path.Join(downloadPath, "data")
 
 		// If the filterDataPath already exists, we must not overwrite
-		// Additionally, if the remote data path doesn't exist, we don't need to do anything
+		// Additionally, if the remote data path doesn't exist, we don't need
+		// to do anything
 		if _, err := os.Stat(localDataPath); err == nil {
-			Logger.Warnf("Filter %s already has data in the 'data' folder. \nYou may manually delete this data and reinstall if you would like these configuration files to be updated.", filterName)
+			Logger.Warnf("Filter %s already has data in the 'data' folder. \n"+
+				"You may manually delete this data and reinstall if you "+
+				"would like these configuration files to be updated.",
+				filterName)
 		} else if _, err := os.Stat(remoteDataPath); err == nil {
 			// Ensure folder exists
 			err = os.MkdirAll(localDataPath, 0666)
 			if err != nil {
-				Logger.Error("Could not create filter data folder", err)
+				Logger.Error("Could not create filter data folder", err) // TODO - I don't think this should break the entire install
 			}
 
 			// Copy 'data' to dataPath
-			if profile.DataPath != "" {
-				err = copy.Copy(remoteDataPath, localDataPath, copy.Options{PreserveTimes: false, Sync: false})
+			if p.DataPath != "" {
+				err = copy.Copy(
+					remoteDataPath, localDataPath,
+					copy.Options{PreserveTimes: false, Sync: false})
 				if err != nil {
-					Logger.Error("Could not initialize filter data", err)
+					Logger.Error("Could not initialize filter data", err) // TODO - I don't think this should break the entire install
 				}
 			} else {
-				Logger.Warnf("Filter %s has installation data, but the dataPath is not set. Skipping.", filterName)
+				Logger.Warnf("Filter %s has installation data, but the "+
+					"dataPath is not set. Skipping.", filterName)
 			}
 		}
 
-		// Check required files
-		file, err := ioutil.ReadFile(downloadPath + "/filter.json")
+		// Create profile from filter.json file
+		remoteProfile, err := LoadFilterJsonProfile(
+			filepath.Join(downloadPath, "filter.json"), *filter)
 		if err != nil {
-			return wrapError(fmt.Sprintf("Couldn't find %s/filter.json!", downloadPath), err)
+			return err // TODO - I don't think this should break the entire install. Just remove the files and continue.
 		}
 
-		// Load subprofile (remote filter)
-		var remoteProfile Profile
-		err = json.Unmarshal(file, &remoteProfile)
-		if err != nil {
-			return wrapError(fmt.Sprintf("Couldn't load %s/filter.json: ", downloadPath), err)
-		}
-		// Propagate venvSlot property
-		for f := range remoteProfile.Filters {
-			remoteProfile.Filters[f].VenvSlot = filter.VenvSlot
-		}
-		// Install dependencies of remote filters
-		// recursion ends when there is no more nested remote dependencies
-		err = InstallDependency(remoteProfile, isForced)
+		// Install dependencies of remote filters. Recursion ends when there
+		// is no more nested remote dependencies.
+		err = remoteProfile.Install(isForced)
 		if err != nil {
 			return err
 		}
 
-		// Install filter dependencies
+		// Install filter dependencies (python/nodejs modules etc.)
+		// TODO - can we move this out of the loop? This way the filters from
+		// config.json would also run their install function and we could
+		// modify the code a little to enable dependencies for them.
 		for _, filter := range remoteProfile.Filters {
 			if filter.RunWith != "" {
 				if f, ok := FilterTypes[filter.RunWith]; ok {
 					err := f.install(filter, downloadPath)
 					if err != nil {
-						return wrapError(fmt.Sprintf("Couldn't install filter %s", filter.Name), err)
+						return wrapError(fmt.Sprintf(
+							"Couldn't install filter %s", filter.Name), err)
 					}
 				} else {
-					Logger.Warnf("Filter type '%s' not supported", filter.RunWith)
+					Logger.Warnf(
+						"Filter type '%s' not supported", filter.RunWith)
 				}
 			}
 		}
 	}
 	return nil
 }
+
+// GetFilterPath returns URL for downloading the filter or empty string
+// if the filter is not remote
+func (f *Filter) GetDownloadUrl() string {
+	if f.Url != "" {
+		return FilterNameToUrl(f.Url, f.Filter)
+	} else if f.Filter != "" {
+		return FilterNameToUrl(StandardLibraryUrl, f.Filter)
+	}
+	return ""
+}
+
+// GetIdName returns the name that identifies the filter. This name is used to
+// create and access the data folder for the filter. This property only makes
+// sense for remote filters. Non-remote filters return empty string.
+func (f *Filter) GetIdName() string {
+	if f.Filter != "" {
+		return f.Filter
+	} else if f.Url != "" {
+		splitUrl := strings.Split(f.Url, "/")
+		return splitUrl[len(splitUrl)-1]
+	}
+	return ""
+}
+
+// Download downloads the filter and returns the download path. If the filter
+// is not remote, it returns empty string.
+func (f *Filter) Download(isForced bool) (string, error) {
+	url := f.GetDownloadUrl()
+	if url == "" {
+		return "", nil // No dependency to install
+	}
+
+	// Download the filter into the cache folder
+	downloadPath := UrlToPath(url)
+
+	// If downloadPath already exists, we don't need to download it again.
+	// Force mode allows overwriting.
+	if _, err := os.Stat(downloadPath); err == nil {
+		if !isForced {
+			Logger.Warnf("Dependency %s already installed, skipping. Run "+
+				"with '-f' to force.", url)
+			return "", nil
+		} else {
+			Logger.Warnf("Dependency %s already installed, but forcing "+
+				"installation.", url)
+			err := os.RemoveAll(downloadPath)
+			if err != nil {
+				return "", wrapError("Could not remove installed filter", err)
+			}
+		}
+	}
+
+	Logger.Infof("Installing dependency %s...", url)
+
+	// Download the filter fresh
+	ok, err := DownloadGitHubUrl(url, downloadPath)
+	if err != nil {
+		Logger.Debug(err)
+	}
+	if !ok {
+		Logger.Debug(
+			"Failed to download filter " + f.Filter + " without git")
+		err := getter.Get(downloadPath, url)
+		if err != nil {
+			return "", err
+		}
+	}
+
+	// Remove 'test' folder, which may be installed via git-getter library
+	// This is a workaround for cases where our own getter library is not
+	// able to download the filter.
+	testFolder := path.Join(downloadPath, "test")
+	if _, err := os.Stat(testFolder); err == nil {
+		os.RemoveAll(testFolder)
+		if err != nil {
+			Logger.Debug("Could not remove test folder", err)
+		}
+	}
+	return downloadPath, nil
+}