bridgetownrb / bridgetown

A next-generation progressive site generator & fullstack framework, powered by Ruby
https://www.bridgetownrb.com
MIT License
1.16k stars 112 forks source link

Thoughts before PR #744

Closed sandstrom closed 1 year ago

sandstrom commented 1 year ago

I noticed this issue: https://github.com/bridgetownrb/bridgetown/issues/681

Basically, it's about output that looks like this if you have a lot of .multi pages and no locale property set in the frontmatter.

[Frontend] node esbuild.config.js --watch
[Bridgetown]        Environment: development
[Bridgetown]             Source: /Users/my-user/repos/org/website/src
[Bridgetown]        Destination: /Users/my-user/repos/org/website/build
[Bridgetown]     Custom Plugins: /Users/my-user/repos/org/website/plugins
[Bridgetown]         Generating… 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
[Bridgetown] key `locales' not found in attributes for new Bridgetown::Model::Base 
…

This is the line that triggers the warning: https://github.com/bridgetownrb/bridgetown/blob/main/bridgetown-core/lib/bridgetown-core/collection.rb#L285

And this is the actual warning being printed out: https://github.com/bridgetownrb/bridgetown/blob/main/bridgetown-core/lib/bridgetown-core/model/base.rb#L154

PR questions

Are you open to changing this?

The two options I think is best:

  1. We could remove the warning altogether. I cannot judge how useful it is to others though.
  2. We could move front-matter access from model.my_key to model.front_matter.retrieve('my_key') or model.retrieve_front_matter('my_key') and then let the retrieve method make warnings configurable, i.e. retrieve(key, unknown_warning: true), and then invoke it with no warning from collection.rb.

(I cannot judge the work for (2) though, but overall I think method_missing is often a source of confusion, and ideally accessing things explicitly is better, but I don't know enough about internals to judge if this is a good idea here)

jaredcwhite commented 1 year ago

@sandstrom Yeah, seems like a mistake. We could just switch from model.locales to model.attributes[:locales] which wouldn't trigger a warning. The warning is for the benefit of users, not for our internal purposes.

sandstrom commented 1 year ago

@jaredcwhite Thanks!

I've opened a PR, I hope it does the job: https://github.com/bridgetownrb/bridgetown/pull/748