Add readFile template func #2008
Conversation
| "readFile": func(i interface{}) (string, error) { | ||
| // TODO(bep): Stupid type assertion, see https://github.com/spf13/afero/issues/76 | ||
| return readFile(afero.NewBasePathFs(hugofs.SourceFs, viper.GetString("WorkingDir")).(*afero.BasePathFs), cast.ToString(i)) | ||
| }, |
There was a problem hiding this comment.
Why can't this happen in readFile? Also, if an inline func can't fit on a single line, I'd prefer to move the complexity to a named func.
There was a problem hiding this comment.
As I said, this is work in progress. This construction is to make it testable (just switch the FS) -- but I wasn't entirely happy about it, so I'm still thinking.
bep
force-pushed
the
readFile
branch
3 times, most recently
from
1d157c2
to
97677b5
Compare
|
Needed by #2010 |
bep
force-pushed
the
readFile
branch
2 times, most recently
from
08b974a
to
7e5635c
Compare
|
@moorereason I have fixed re. your comment ... but the fix introduced some other changes :-) |
bep
force-pushed
the
readFile
branch
2 times, most recently
from
cfb862c
to
7cde8e6
Compare
|
@spf13 do you have any objections/input on this one? |
|
@bep, |
This PR depends on that to compile, it IS the core of this PR -- and reimplementing the logic that already exists in Afero isn't a path I'm going. I can try to split the commits it it's not too much work. The Afero filesystems are brilliant. Hugo's way of adding a dozen of global variables for the different filesystems was a mistake. In |
This also includes a refactor of the hugofs package and its usage.
The motivation for that is:
The Afero filesystems are brilliant. Hugo's way of adding a dozen of global variables for the different filesystems was a mistake. In readFile (and also in some other places in Hugo today) we need a way to restrict the access inside the working dir. We could use ioutil.ReadFile and implement the path checking, checking the base path and the dots ("..") etc. But it is obviously better to use an Afero BasePathFs combined witha ReadOnlyFs. We could create a use-once-filesystem and handle the initialization ourselves, but since this is also useful to others and the initialization depends on some other global state (which would mean to create a new file system on every invocation), we might as well do it properly and encapsulate the predefined set of filesystems. This change also leads the way, if needed, to encapsulate the file systems in a struct, making it possible to have several file system sets in action at once (parallel multilanguage site building? With Moore's law and all...)
Fixes gohugoio#1551
|
@bep This looks great. The addition of the helper hugofs.Init* functions is really nice and clean. I agree with @moorereason that this is really two commits, 1. the hugofs cleanup, 2. the readFile function. If it's not a lot of trouble it would be cleaner to have two commits (still one PR). I'm not sure I totally understand why your preference for Function & SetFunction vs Variable. I don't have a problem with either approach, but find them very similar and only semantically different. Can you help me to understand better why you felt the Function approach was better here? Feel free to split and merge or just merge if it's tough to split. I also like readFile and think it's a great idea and really powerful when combined with things like markdownify, etc. I think the name works well also. |
Variables access cannot have any side effects. And the names are better. You could argue that We've had a few subtle but serious bugs where we had a mix of different file systems in play. The tests have been setting the source file system global, but didn't care too much about the destination ... In the site build chain, the file systems have been initialized at some point, then much later, depending on a flag or some other magic, the source system has been changed to something else ... making it hard to look at some file operation and decide: Will that file be read from disk? I'm all for the pragmatic approach of using global variables if it makes the life much easier or if it is a resource that is static by nature (one should think file systems fall into the latter category, but Afero changed that) -- in Hugo we've taken it a little bit too far: MainSite, Viper, SourceFs, DestinationFS, Template ... Other? Not too bad, but it makes the code hard too follow and the tests a little bit shaky and maybe not perfectly realistic at all times. |
|
@bep |
digitalcraftsman
force-pushed
the
master
branch
from
49b4f8e
to
93e41a1
Compare
|
@spf13 keywords here are also maybe encapsulation and immutability. With the change in this PR the access to the file systems are encapsulated, but they are still mutable, however in a more controlled fashion. |
|
Merged in 4f66f79 |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is Work in progress, but works fine. Would love input on the name, but I have gone for simplicity, so it is all or nothing (no read line 10-20 etc.).