Improves project file detection #220

DavidSeptimus-Klotho · 2023-02-09T18:37:59Z

This PR supports #105

Renamed "package" to "project" in ReadDir()to move away from NodeJS terminology
Replaced fixed project file names with predicate matching + a description for logging
Implemented .csproj file detection for C#
Implemented error handling for handling in the event multiple project files of the same language are detected in the same directory

Standard checks

Unit tests: Any special considerations?
Docs: Do we need to update any docs, internal or public?
Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working?

- Renamed "package" to "project" to move away from NodeJS terminology - Replaced fixed project file names with predicate matching + a description for logging - Implemented .csproj file detection for C#

jhsinger-klotho · 2023-02-10T00:26:34Z

pkg/input/readdir.go

+				f, err = addFile(fsys, path, relPath, lang.projectFileOpener)
+				lang.foundProjectFile = true
+				isProjectFile = true
+				projectDirs[lang.name] = append(projectDirs[lang.name], prjDir)


is this also enabling the ability to have multiple languages in the same klotho compilation? Or what is projectDirs alluding to?

projectDirs refers to directories in which we've detected project files for each supported language. For the moment it's used to enable language-specific file/directory exclusions, and to ensure that compilation fails if two project files exist for the same language in the same directory.

Support for multiple languages in a single compilation is not specifically handled here. The executable plugins don't currently support multiple languages in a single execution unit.

jhsinger-klotho · 2023-02-10T00:26:51Z

just some lint errors as well

gordon-klotho

Linter caught some things that need to be fixed.

gordon-klotho · 2023-02-10T15:19:14Z

pkg/input/readdir.go

+	}
+}
+
+func hasSuffix(expected string) predicate.Predicate[string] {


IMO this should probably be hasExtension and use the filepath.Ext(name) to test. In this case it's close to functionally the same, but I think it makes the intention clearer. There's a few edge cases that it'd catch too (like the file who's name is .csproj)

gordon-klotho · 2023-02-10T15:20:45Z

pkg/input/readdir.go

-
+			case "bin", "obj":
+				dir, _ := filepath.Split(info.Name())
+				for _, projectDir := range projectDirs[CSharp] {


This is susceptible to iteration order problems. What if the csproj was found after the bin/obj directories?

You're right. I got the wrong impression regarding iteration order since these directories are lowercase and the .csproj file I was testing had an uppercase name.

An alternative might be to instead just look at the peers of bin/obj and see if there's a *.csproj file. So you don't need to read it beforehand.

gordon-klotho · 2023-02-10T17:09:39Z

pkg/input/readdir.go

-				lang.foundPackageFile = true
-				isPackageFile = true
+			if lang.projectFilePredicate != nil && lang.projectFilePredicate(info.Name()) {
+				prjDir, _ := filepath.Split(info.Name())


Suggested change

prjDir, _ := filepath.Split(info.Name())

prjDir := filepath.Dir(info.Name())

gordon-klotho · 2023-02-14T16:25:33Z

pkg/input/readdir.go

+func hasExtension(expected string) predicate.Predicate[string] {
+	extension := "." + expected
+	return func(name string) bool {
+		return extension != name && strings.HasSuffix(name, extension)


Suggested change

return extension != name && strings.HasSuffix(name, extension)

return filepath.Ext(name) == extension

gordon-klotho · 2023-02-14T16:25:57Z

pkg/input/readdir.go

+}
+
+func hasExtension(expected string) predicate.Predicate[string] {
+	extension := "." + expected


IMO require the caller to include the . which matches prior art with filepath.Ext

gordon-klotho · 2023-02-14T17:18:20Z

pkg/input/readdir.go

 	}
 	yamlLang := &languageFiles{name: Yaml}
 	dockerfileLang := &languageFiles{name: DockerFile}
-	allLangs := []*languageFiles{jsLang, pyLang, goLang, yamlLang}
+	allLangs := []*languageFiles{jsLang, pyLang, goLang, yamlLang, csLang}
+	projectDirs := map[languageName][]string{}


What's projectDirs used for? Seems like it's only used to skip folders. Seems like a lot of complication compared to just processing them normally. At least for now seems like premature optimization unless there's something you've already run into (in which case, can you document those cases in the comments?).

gordon-klotho · 2023-02-14T17:19:07Z

pkg/input/readdir.go

-
+			case "bin", "obj":
+				dir, _ := filepath.Split(info.Name())
+				for _, projectDir := range projectDirs[CSharp] {


An alternative might be to instead just look at the peers of bin/obj and see if there's a *.csproj file. So you don't need to read it beforehand.

pkg/input/readdir.go

gordon-klotho · 2023-02-15T14:08:43Z

pkg/input/readdir.go

+				projectFile, err = func() (core.File, error) {
+					defer f.Close()
+					return lang.projectFileOpener(prjFilePath, f)
+				}()


This isn't really idiomatic for Go. Something more like

projectFile, err := lang.projectFileOpener(prjFilePath, f) f.Close() if err != nil { // ... }

github-actions · 2023-02-16T17:11:04Z

Package	Line Rate	Health
github.com/klothoplatform/klotho/pkg/analytics	2%	❌
github.com/klothoplatform/klotho/pkg/annotation	23%	❌
github.com/klothoplatform/klotho/pkg/cli	4%	❌
github.com/klothoplatform/klotho/pkg/core	21%	❌
github.com/klothoplatform/klotho/pkg/env_var	82%	✔
github.com/klothoplatform/klotho/pkg/exec_unit	54%	➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes	59%	➖
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm	39%	❌
github.com/klothoplatform/klotho/pkg/input	72%	➖
github.com/klothoplatform/klotho/pkg/lang	38%	❌
github.com/klothoplatform/klotho/pkg/lang/dockerfile	0%	❌
github.com/klothoplatform/klotho/pkg/lang/golang	33%	❌
github.com/klothoplatform/klotho/pkg/lang/javascript	48%	❌
github.com/klothoplatform/klotho/pkg/lang/python	61%	➖
github.com/klothoplatform/klotho/pkg/lang/yaml	0%	❌
github.com/klothoplatform/klotho/pkg/logging	6%	❌
github.com/klothoplatform/klotho/pkg/multierr	95%	✔
github.com/klothoplatform/klotho/pkg/provider/aws	60%	➖
github.com/klothoplatform/klotho/pkg/runtime	78%	✔
github.com/klothoplatform/klotho/pkg/static_unit	33%	❌
github.com/klothoplatform/klotho/pkg/validation	73%	➖
github.com/klothoplatform/klotho/pkg/yaml_util	79%	✔
Summary	42% (4035 / 9716)	❌

Improves project file detection

791d839

- Renamed "package" to "project" to move away from NodeJS terminology - Replaced fixed project file names with predicate matching + a description for logging - Implemented .csproj file detection for C#

jhsinger-klotho reviewed Feb 10, 2023

View reviewed changes

gordon-klotho reviewed Feb 13, 2023

View reviewed changes

Fixes project directory detection

c08da13

DavidSeptimus-Klotho force-pushed the dynamic-project-file-detection branch from c4d7f77 to c08da13 Compare February 13, 2023 23:17

DavidSeptimus-Klotho added 2 commits February 13, 2023 16:43

Changes hasSuffix to hasExtension

b49890a

Fixes ineffectual assignment

6d15bca

jhsinger-klotho approved these changes Feb 14, 2023

View reviewed changes

gordon-klotho reviewed Feb 15, 2023

View reviewed changes

DavidSeptimus-Klotho added 2 commits February 16, 2023 10:05

Implements requested changes

5258ab3

Implements requested changes

733d23a

DavidSeptimus-Klotho mentioned this pull request Feb 16, 2023

Adds initial support for C# execution unit + expose #252

Merged

gordon-klotho approved these changes Feb 20, 2023

View reviewed changes

DavidSeptimus-Klotho merged commit 8bab35f into main Feb 20, 2023

DavidSeptimus-Klotho deleted the dynamic-project-file-detection branch February 20, 2023 20:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improves project file detection #220

Improves project file detection #220

DavidSeptimus-Klotho commented Feb 9, 2023

jhsinger-klotho Feb 10, 2023

DavidSeptimus-Klotho Feb 13, 2023

jhsinger-klotho commented Feb 10, 2023

gordon-klotho left a comment

gordon-klotho Feb 10, 2023

gordon-klotho Feb 10, 2023

DavidSeptimus-Klotho Feb 13, 2023 •

edited

Loading

gordon-klotho Feb 14, 2023

gordon-klotho Feb 10, 2023

gordon-klotho Feb 14, 2023

gordon-klotho Feb 14, 2023

gordon-klotho Feb 14, 2023

gordon-klotho Feb 14, 2023

gordon-klotho Feb 15, 2023

github-actions bot commented Feb 16, 2023

	prjDir, _ := filepath.Split(info.Name())
	prjDir := filepath.Dir(info.Name())

	return extension != name && strings.HasSuffix(name, extension)
	return filepath.Ext(name) == extension

Improves project file detection #220

Improves project file detection #220

Conversation

DavidSeptimus-Klotho commented Feb 9, 2023

Standard checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhsinger-klotho commented Feb 10, 2023

gordon-klotho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidSeptimus-Klotho Feb 13, 2023 • 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

github-actions bot commented Feb 16, 2023

DavidSeptimus-Klotho Feb 13, 2023 •

edited

Loading