Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion for solution #476. #810

Closed
wants to merge 1 commit into from
Closed

Suggestion for solution #476. #810

wants to merge 1 commit into from

Conversation

klakegg
Copy link
Contributor

@klakegg klakegg commented Jan 20, 2015

Reads files inside data/ and makes data available in .Site.Data. May need some refactoring.

@bep
Copy link
Member

bep commented Jan 20, 2015

Hei og takk for sist!

No har ein også #748 -- men løysinga di er både enkel og fleksibel mht. format. Det blir kanskje heller eit spørsmål om "globale data" eller data som høyrer til ei side.

Kan du fjerne den utkommenterte seksjonen og gjere ein "push -f" (overskrive innsjekkinga di)?

To others: Sorry about the nynorsk language.

@klakegg
Copy link
Contributor Author

klakegg commented Jan 21, 2015

Verden er liten. ;)

Første innsjekk er overskrevet.

And to the english version...
I thing a "global data" solution is wanted in #476, and it's the most appealing solution to me. A "page data" solution could be realized by creating a special directory inside the data directory.

@@ -332,9 +373,10 @@ func (s *Site) initialize() (err error) {
}

staticDir := helpers.AbsPathify(viper.GetString("StaticDir") + "/")
dataDir := helpers.AbsPathify(viper.GetString("StaticDir") + "/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> DataDir

@bep
Copy link
Member

bep commented Jan 21, 2015

This should work and is simple enough, @spf13 ?

It should support sub-folders, but that's simple to add.

@spf13
Copy link
Contributor

spf13 commented Jan 21, 2015

Looks really good. It's pretty much exactly like how I would do it (except it needs tests ;) ) .

About sub directories, where would they appear? .Site.Data[subdir][filename] seems like the obvious place to me.

@bep
Copy link
Member

bep commented Jan 21, 2015

.Site.Data[subdir][filename] is clever -- then it's easy to iterate over all data files for a given subdir, but I suggest using path (i.e. Unix style slashes) and not filepath for the subdir key.

@klakegg do you have any time to spare to take a stab at adding support for subfolders? There are examples of using the filepath.Walk some other places in the Hugo code base. Note the ERROR logging on symbolic links -- this might be good to copy over, as this seems to confuse users.

Also some tests would be cool!

@klakegg
Copy link
Contributor Author

klakegg commented Jan 21, 2015

I think using Filesystem is better because of less duplication (symlinks) and included caching.

Files in data/ are imported before data in subfolders allowing overriding using subfolders.

@bep
Copy link
Member

bep commented Jan 21, 2015

Filesystem vs Walk, I don't care.

But overriding in sub-folders will get confusing pretty fast.

  • I think people would expect to be able to create more than one level of subdirs and address the data items in a way that matches that directory structure
  • Ideally something like this: /data/sub1/sub2/mydata.json ==> {{ sub1.sub2.mydata.mykey }}

Any thoughts @spf13 ?

@klakegg
Copy link
Contributor Author

klakegg commented Jan 21, 2015

If /data/sub1/sub2.json sets key 'mydata.mykey' to 'foo', and you have a file /data/sub1/sub2/mydata.json setting 'mykey' to 'bar'. What to you expect .Site.Data.sub1.sub2.mydata to return?

Btw, now also with test. ;)

@bep
Copy link
Member

bep commented Jan 21, 2015

In that case I would expect /sub2/mydata.json's value, but that isn't override as a feature -- I would even consider returning an error for these duplicates so the user can do some renaming.

@bep
Copy link
Member

bep commented Jan 21, 2015

Oh, and the failing test is most likely due to Go maps' random iteration order.

@klakegg
Copy link
Contributor Author

klakegg commented Jan 22, 2015

The implementation is now changed to support any amount of subfolders. The test is still unstable (not changed) and will be updated if this is the solution we want.

@bep
Copy link
Member

bep commented Jan 25, 2015

There is some spring code cleaning to be done here, but:

  • It's simple enough, and after some limited tests, It does what I would expect.
  • I might tone down the duplicate check to just log an ERROR and not STOP -- but that's details

So, @spf13 - to sum up (with my special $ syntax):

/data/my.json$mkey:1234 => {{ .Data.my.mkey }} ==> 1234
/data/sub1/my.json$mkey:1234 => {{ .Data.sub1.my.mkey }} ==> 1234
/data/sub1.json$mkey:1234 => {{ .Data.sub1.my.mkey }} ==> boom ERROR

It wold need documentation and tests, but from a bird's perspective, it looks correct, right?

@spf13
Copy link
Contributor

spf13 commented Jan 25, 2015

@bjornerik I don't follow the special $ syntax. Does that just mean the files content after it?

I think we are heading in a very good direction.

I think duplication should just log an ERROR.

@bep
Copy link
Member

bep commented Jan 25, 2015

the $ syntax is just there to separate the file (my.json) with the key inside the file (mkey). It's just for this discussion and not part of any API.

@spf13
Copy link
Contributor

spf13 commented Jan 25, 2015

Ok. I think this looks really good. We should fix the tests, merge it, and add documentation.

@bep bep added this to the v0.13 milestone Jan 25, 2015
@bep
Copy link
Member

bep commented Jan 25, 2015

@klakegg could you fix/add some tests and doc? And replace the error-return with just ERROR logging

@bep
Copy link
Member

bep commented Jan 28, 2015

@klakegg bump. It isn't that busy in the government :-) If this is going into the 0.13 release it needs docs/tests -- and I'm too lazy/busy doing it myself.

@bep bep self-assigned this Feb 6, 2015
@bep
Copy link
Member

bep commented Feb 6, 2015

OK, I need this one, so I'll finish it.

@bep bep mentioned this pull request Feb 8, 2015
@bep
Copy link
Member

bep commented Feb 8, 2015

Replaced by #885

@bep bep closed this Feb 8, 2015
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants