fix: broken embedio on windows #1135
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## gh-windows #1135 +/- ##
=============================================
Coverage ? 83.14%
=============================================
Files ? 164
Lines ? 7688
Branches ? 0
=============================================
Hits ? 6392
Misses ? 1042
Partials ? 254
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
internal/io/embedio.go
Outdated
| @@ -0,0 +1,15 @@ | |||
| package io | |||
There was a problem hiding this comment.
This is missing our license header
There was a problem hiding this comment.
Also, having a whole package for a single function looks like an overkill
There was a problem hiding this comment.
The io package already exists. The other files did not seem appropriate in this package, so I choose a new. If you wish a different approach, then please give a more detailed description of what you have in mind.
internal/io/ioutil.go
Outdated
| ) | ||
|
|
||
| // FSReadFile wraps fs.ReadFile supporting embedio on windows | ||
| func FSReadFile(fsys fs.FS, name string) ([]byte, error) { |
There was a problem hiding this comment.
wonder why io and not fs package since it wraps a fs function
There was a problem hiding this comment.
I would not change the FSReadFile for non windows, instead I would define a global variable for this function, use default one for non windows systems and use a custom one for windows (under a build tag, you can use _windows.go suffix swell) and then enable windows on CI.
Since several changes will happen before windows CI is green I would create a branch windows_support and merge all this PRs against that branch.
All that said, thanks a lot for all this effort!
|
@jcchavezs On non windows systems the if clause is optimized away by the compiler and on windows it is just always used (filepath.Seperator is already a const). Using a variable and build tags would make it more complicated in my opinion. i used the internal/io package, because it already existed. I don't see any internal/fs package. |
|
Despite the performance impact, I'd rather not add ifs that pollutes the code and instead abstract that off into a os dependent file. |
|
@jcchavezs done |
|
Please rebase your PR on top of |
At the moment loadFromFile and parser.FromFile fail on windows if fs.Sub filesystem is used. This is due to the different behaviour related to filepath.Seperator and absolute paths.
This patch fixes TestEmbedFS, TestLoadFromFileAbsolutePath and TestLoadFromCustomFS on windows.
As loadFromFile and Parser.FromFile are not related to each other, it made more sense to me, to put the common code into the io package.