make embed assets relative to where annotation lives or abs from proj… #30
Conversation
pkg/exec_unit/plugin_assets.go
Outdated
| func (m *assetPathMatcher) ModifyPathsForAnnotatedFile(path string) error { | ||
| newInclude := []string{} | ||
| for _, pattern := range m.include { | ||
| absPath, err := filepath.Abs(pattern) |
There was a problem hiding this comment.
Seems odd to convert the pattern to an absolute path, perhaps IsAbs would be better?
Also, do we expect these paths to be standard unix or platform-specific? These calls will all be platform specific. If platform specific is intended, the TrimPrefix later on the leading slash won't work on Windows.
There was a problem hiding this comment.
IsAbs is probably better
so youre saying we will see the path for our files as static/file on mac or linux and /static/file on windows?
If so then why dont we convert everything to an absolute path from the klotho root in readdir so we have a standard path we can follow? Our internal compiler logic shouldnt be dependent on the platform we are running on.
pkg/exec_unit/plugin_assets.go
Outdated
| absPath, err := filepath.Abs(pattern) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if absPath == pattern { | ||
| newExclude = append(newExclude, strings.TrimPrefix(pattern, "/")) | ||
| continue | ||
| } | ||
| relPath, err := filepath.Rel(filepath.Dir("."), filepath.Join(filepath.Dir(path), pattern)) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
nit: Seems like this could be extracted into a reusable function.
pkg/exec_unit/plugin_assets.go
Outdated
| @@ -75,6 +84,45 @@ type assetPathMatcher struct { | |||
| err error | |||
| } | |||
|
|
|||
| func (m *assetPathMatcher) ModifyPathsForAnnotatedFile(path string) error { | |||
There was a problem hiding this comment.
nit: Having this be a mutator function seems a bit weird to me. An assetPathMatcher isn't valid before calling this function and I'm not sure if it'd behave right if called twice, so callers need to make sure to call it exactly once on construction. Seems like it might be better served in a NewAssetPathMatcher constructor function.
pkg/exec_unit/plugin_assets.go
Outdated
| if filepath.IsAbs(path) { | ||
| return strings.TrimPrefix(path, "/"), nil | ||
| } | ||
| relPath, err := filepath.Rel(filepath.Dir("."), filepath.Join(filepath.Dir(currentPath), path)) |
There was a problem hiding this comment.
nit: I think the Rel might be redundant since the currentPath is always an input file path (where the annotation was specified) which is always relative to '.'. I'm not sure why it was there originally.
There was a problem hiding this comment.
Your right, just pushed up a change which makes the logic much simpler
|
…ect root
• Does any part of it require special attention?
• Does it relate to or fix any issue? closes #578
make embed assets relative to where annotation lives or abs from project root
Added docs in pr linked below
Standard checks