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

Resource.Name should (also) contain any subdir for page resources #4295

Closed
bep opened this Issue Jan 18, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@bep
Copy link
Member

bep commented Jan 18, 2018

Which was a breaking change in Hugo 0.33 -- and a bad idea. This is used in GetByPrefix etc.

@bep bep added the Bug label Jan 18, 2018

@bep bep added this to the v0.33.1 milestone Jan 18, 2018

@bep

This comment has been minimized.

@regisphilibert

This comment has been minimized.

Copy link

regisphilibert commented Jan 18, 2018

Will this also change the current metadata targeting using src? For now, it only works if subdir is included in the request.
src: images/sunset.jpg
src: */sunset.jpg
src: sunset.jpg 🚫

I like it as is. It makes sense.

@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 18, 2018

Will this also change the current metadata targeting using src?

Yes. There should be a symmetry here, I think. But this isn't carved in stone.

@regisphilibert

This comment has been minimized.

Copy link

regisphilibert commented Jan 18, 2018

Too bad, the current behaviour made a lot of sense considering that the key chosen is 'src' rather than 'filename' or 'basename'. Intuitively we're expecting it to be matched with the src of the image relative to the bundle.
Plus it is was nice feat to be able to easily batch metadata every files in a directory. You could target all the files you deliberately put in the image directory regardless of their filename/extension.

But we would need /**/*.jpg to also work and I didn't check that. Otherwise a user won't be able to target files matching an extension if they don't know the dir structure of the bundle...

@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 18, 2018

I will think a little before doing...

@kaushalmodi

This comment has been minimized.

Copy link
Member

kaushalmodi commented Jan 19, 2018

I believe I hit this same issue while writing a test case here. Note how in the first src in that example, I needed to hard-code the image/ sub-directory.. */copy-of-*.png would not work.


Update: I apologize, */copy-of-*.png does work.. I must have tried *copy-of-*.png (note the missing /) earlier, which does not work.

@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 19, 2018

OK, I have slept on this, and I'm not convinced that the current solution is so far off. It was unintended, and it has some inconsistency in it ... but. We need to think this through before committing.

Given this bundle:

my-bundle
├── a.jpg
├── b.jpg
├── holiday-photos
│   ├── a.jpg
│   ├── b.jpg
│   └── c.jpg
├── index.md
├── logos
│   ├── a.jpg
│   └── b.jpg
├── notes
│   ├── notes1.md
│   └── notes2.md
└── notes.md

The ambigous naming is intended, but real enough with the image processing software of today.

The default value of Name (used in both the Prefix methods and the src matching) are:

  • For the content files it is (if my memory serves me right) "notes", "notes1" etc. No directory name. This is inconsistent
  • For the images, Name is the relative path (Unix style slashes) to the file: a.jpg (root), holiday-photos/a.jpg.

My thoughts about this are:

  1. We need to find the solution that fits best (there is no perfect solution).
  2. Most people have everything in one folder. And if not:
  3. There is a thought behind how you organize your images. There is a reason why you want to split them into folders, you don't want them to end up in the same basket.

In the above example, there is (almost) never a situation where I want to use the holiday photos and logos in the same collection.

I most likely want to do this:

src = " holiday-photos/*.*"
title = "Holiday Photo #:counter"
name = "my-holiday-photo-:counter"

And in the template:

{{ (.ByType "images").ByPrefix "my-holiday" }}

Or something like that (I deliberately made it a little stupid). And similar with the logos. If you don't want to define any name in front matter, to get the holiday photos only:

{{ ByPrefix "holiday-photos" }}

The above would not be possible with Hugo 0.32.

So, my proposal is this:

  • We fix the Name inconsistency for the content files (also include sub-folders in the name for them)
  • If @kaushalmodi is right about the */*.jpg matching not working, we try to find a fix for that -- but that is not a deal breaker for this particular issue.
@regisphilibert

This comment has been minimized.

Copy link

regisphilibert commented Jan 19, 2018

I agree 100% on the need for something like what you described above and just wonder if ‘ByPrefix’ is the right method for that. While we’re just innocently chatting, hear me out.

1/ I wonder if the notion of prefix really apply anymore ? Again the only graps I have of this notion is how it was used in Hugo, (for me it’s just the beginning of a string) so feel free to educate me.
2/ Now that the user has been introduced to the pattern matching potential of the metadata src param. Shouldn’t he/she expect from the package a method at least equally powerful?

Would it be too far of as to suggest the keeping of .ByPrefix as was in .32, and the introduction of a new method (don’t mind the naming):

{{ .Resources.ByPath "images-what/*" }}
{{ .Resources.ByPath "*/*.jpg" }}

This would bring symmetry only not on ByPrefix.

I don’t want to complexifie the resource package.

I really appreciate its current state, I just happen to be really excited by the newly introduced « glob like » pattern matching of « src » and realize ByPrefix/GetByPrefix uses a less powerful pattern matching leaving the package less equipped than its Front Matter’s metadata logic.

Feel free counter, question, or shrug off.

@kaushalmodi

This comment has been minimized.

Copy link
Member

kaushalmodi commented Jan 19, 2018

@bep

If @kaushalmodi is right about the */*.jpg matching not working

Hmm, that does work. Sorry for the confusion.. I must have missed out on that forward slash earlier.


For the record, I have the test cases here ran on Hugo v0.33 (following the example given by @regisphilibert here):

  • src "images/copy-of-*.png" Markdown, Hugo output
  • src "*/copy-of-*.png" Markdown, Hugo output
  • src "copy-of-*.png" 🚫Markdown, Hugo outputNotice in the Resources section on this page that the image title does not show up as set in the title in the resources front-matter.
@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 19, 2018

@regisphilibert finding files/folders by prefix is very common and easy to understand -- it fits nicely with the directory structure and it solves 90% of the use cases (guesswork).

And this issue is not about adding more methods. This is a bug fix for a release on Monday. Nothing new.

We also have where to do advanced queries.

@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 19, 2018

But note that I have no problem adding a Resources.ByMatch (or something) that uses shell file name pattern same as in that src thing (but that is another issue and Hugo 0.34 meat).

But that method should use the same value to match against as the existing ByPrefix. And I'm leaning towards how it currently works for images.

@regisphilibert

This comment has been minimized.

Copy link

regisphilibert commented Jan 19, 2018

finding files/folders by prefix is very common and easy to understand -- it fits nicely with the directory structure and it solves 90% of the use cases (guesswork).

Thanks, that's the information I lacked.

But note that I have no problem adding a Resources.ByMatch (or something) that uses shell file name pattern same as in that src thing (but that is another issue and Hugo 0.34 meat).

Awesome, will create another issue for your consideration.

But that method should use the same value to match against as the existing ByPrefix. And I'm leaning towards how it currently works for images.

Agree.

Thanks for your answers.

@bep

This comment has been minimized.

Copy link
Member Author

bep commented Jan 19, 2018

And to add to the motivation of keeping the Prefix* Methods: It will be faster than the Match variant. Probably not important in most projects.

@bep bep changed the title Resource.Name contains subdir for images Resource.Name should (also) contain any subdir for page resources Jan 20, 2018

bep added a commit to bep/hugo that referenced this issue Jan 20, 2018

@bep bep closed this in #4304 Jan 21, 2018

bep added a commit that referenced this issue Jan 21, 2018

@bep bep modified the milestones: v0.33.1, v0.34 Jan 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.