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

Cleanups to Resolver/UnboundTemplate details #41997

Merged
merged 3 commits into from
Apr 20, 2021

Conversation

jhawthorn
Copy link
Member

This PR includes cleanup of parsed details handling the towards a redo of #39362, which I never completed.

  • Introduces a Struct to store the parsed details instead of using a hash (as suggested in Use PathParser for finding templates too  #39362).
  • Parses and stores the full set of details on UnboundTemplate instead of treating it as just an options hash to pass to Template. This will allow filtering later.
  • Determines virtual_path from the filesystem instead of from the requested path

Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

Cool! Feels a little odd how often we're passing all the details around instead of generally extracting a Template::Details object, but that could be later, or it doesn't make sense etc.

@jhawthorn
Copy link
Member Author

jhawthorn commented Apr 19, 2021

Feels a little odd how often we're passing all the details around instead of generally extracting a Template::Details object, but that could be later, or it doesn't make sense etc.

I agree it's a bit odd. Complicating it is that we have details in many forms: parsed from a filename, the details requested by the user (arrays for each key), a plain Object representing the requested details, and finally the handler class used by Template. I think a class (or pair of classes for requested vs found) would make sense for that, but I'd like to see what refactoring shakes out first.

Previously we just stored handler, format, and variant and assigned a
default format if none existed.

Now we want to also store locale, and move the default format behaviour
into unbound template.
Previously, it was possible for these not to match due to providing
templates with .'s or using fallback templates.

There is now an exact 1:1 between templates on disk and virtual path.
@jhawthorn jhawthorn merged commit bc1bc32 into rails:main Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants