Refactor front matter handling and extract behavior into loaders #778

michaelherold · 2023-07-03T20:12:05Z

This is a 🙋 feature or enhancement.

Summary

This change reorganizes the front matter-related code so it is fully contained within the Bridgetown::FrontMatter module. That makes it easier to find the functionality and standardizes the way that Bridgetown refers to front matter.

It also adds deprecated constant proxies for backwards-compatibility. It's unclear whether the moved modules were public API or not so the proxies make it so updating will not break anyone's site without warning.

This change also refactors the loading of front matter into a series of classes. This is a first step toward making front matter loaders pluggable to allow, for example, TOML- or KDL-based front matter.

Context

This accomplishes much of the "open to extension" goal in #777.

michaelherold

This is a high-level review of things that I thought of while working on this. It's been several months in the making and I wanted to get it up for review before working through it anymore.

michaelherold · 2023-07-03T20:46:58Z

bridgetown-core/lib/bridgetown-core.rb

+  FrontmatterDefaults = ActiveSupport::Deprecation::DeprecatedConstantProxy.new(
+    "FrontmatterDefaults",
+    "Bridgetown::FrontMatter::Defaults"
+  )
+
+  FrontMatterImporter = ActiveSupport::Deprecation::DeprecatedConstantProxy.new(
+    "FrontMatterImporter",
+    "Bridgetown::FrontMatter::Importer"
+  )


comment: I kept these to honor the move fast but try not to break things principle.

question: These don't use the Bridgetown::Deprecator. Do we want to refactor to use it?

michaelherold · 2023-07-03T20:47:44Z

bridgetown-core/lib/bridgetown-core/front_matter/loaders.rb

+      autoload :Ruby, "bridgetown-core/front_matter/loaders/ruby"
+      autoload :YAML, "bridgetown-core/front_matter/loaders/yaml"
+
+      Result = Struct.new(:content, :front_matter, :line_count, keyword_init: true)


comment: This should likely be marked private. I believe it should be a temporary thing.

michaelherold · 2023-07-03T20:49:52Z

bridgetown-core/lib/bridgetown-core/front_matter/loaders.rb

+      # Registers a new type of front matter loader
+      #
+      # @param loader_class [Loader::Base]
+      # @return [void]
+      def self.register(loader_class)
+        registry.push(loader_class)
+      end
+
+      private_class_method def self.registry
+        @registry ||= []
+      end
+
+      register YAML
+      register Ruby


comment: This is at MVP stage and it's what I'm least excited about. If you notice, I register YAML before Ruby. This is because it needs to be higher precedence to maintain the original business logic of the read_from_matter method.

I didn't want to overengineer here, but I think having both (1) symbol-based registration and (2) a priority level would be useful here. That way people can override by type and give higher precedence to a loader if needed.

Thoughts?

michaelherold · 2023-07-03T20:51:17Z

bridgetown-core/lib/bridgetown-core/front_matter/loaders/base.rb

+      class Base
+        # @param origin_or_layout [Bridgetown::Model::RepoOrigin, Bridgetown::Layout]
+        def initialize(origin_or_layout)
+          @origin_or_layout = origin_or_layout


comment: I don't know if there's a higher-level concept that these things both have in common. Is there?

michaelherold · 2023-07-03T20:52:07Z

bridgetown-core/lib/bridgetown-core/reader.rb

@@ -69,7 +69,7 @@ def read_directories(dir = "")
        file_path = @site.in_source_dir(base, entry)
        if File.directory?(file_path)
          entries_dirs << entry
-        elsif Utils.has_yaml_header?(file_path) || Utils.has_rbfm_header?(file_path)
+        elsif FrontMatter::Loaders.front_matter?(file_path)


question: Would it be worth exposing this method on FrontMatter instead?

michaelherold · 2023-07-03T20:53:36Z

bridgetown-core/test/test_generated_site.rb

@@ -16,7 +16,7 @@ class TestGeneratedSite < BridgetownUnitTest
    end

    should "ensure post count is as expected" do
-      assert_equal 51, @site.collections.posts.resources.size
+      assert_equal 52, @site.collections.posts.resources.size


comment: This test is fragile. Should we remove it?

michaelherold · 2023-07-03T20:53:59Z

bridgetown-core/test/test_ruby_helpers.rb

@@ -21,7 +21,7 @@ def setup
    end

    should "return accept objects which respond to url" do
-      assert_equal "<a href=\"/2020/09/10/further-nested/\">Label</a>", @helpers.link_to("Label", @site.collections.posts.resources.first)
+      assert_equal "<a href=\"/2023/06/30/ruby-front-matter/\">Label</a>", @helpers.link_to("Label", @site.collections.posts.resources.first)


comment: This test is fragile as well. Does it pay for itself?

michaelherold · 2023-07-03T20:54:20Z

bridgetown-core/test/test_utils.rb

@@ -387,20 +387,26 @@ class TestUtils < BridgetownUnitTest
  end

  context "The `Utils.has_yaml_header?` method" do
-    should "accept files with YAML front matter" do


comment: These tests move into the YAML loader tests.

michaelherold · 2023-07-03T20:55:20Z

bridgetown-routes/lib/bridgetown-routes/code_blocks.rb

@@ -30,7 +30,7 @@ def eval_route_file(file, file_slug, app) # rubocop:disable Lint/UnusedMethodArg

          code = File.read(file)
          code_postmatch = nil
-          ruby_content = code.match(Bridgetown::FrontMatterImporter::RUBY_BLOCK)
+          ruby_content = code.match(Bridgetown::FrontMatter::Loaders::Ruby::BLOCK)


comment: This appears to be dealing with front matter without really stating that fact. Perhaps we should refactor it?

michaelherold · 2023-07-03T20:55:45Z

bridgetown-website/src/_docs/plugins/front-matter-loaders.md

+1. `HEADER` matches the opening line of the front matter
+2. `BLOCK` matches the contents of the front matter block with the first capturing group being the content and the regular expression consuming the ending delimiter


question: Is this worth mentioning?

jaredcwhite · 2023-07-31T02:12:13Z

Apologies I haven't had a chance to dig into this @michaelherold, hope to soon. Thanks for working on this as it's an idea that's come up before. In fact, I could foresee relocating all of the front matter logic out to a separate gem (which then is a dependency of core), but probably it's fine to keep it in house for now (though more standalone).

michaelherold · 2023-08-04T02:26:20Z

As I was working on this, I imagined having a gem that Bridgetown would use, so we're thinking along the same lines!

This change refactors the loading of front matter into a series of classes. This is a first step toward making front matter loaders pluggable to allow, for example, [TOML][1]- or [KDL][2]-based front matter. [1]: https://toml.io [2]: https://kdl.dev/

These were developed prior to FrontMatterImporter and are no longer used.

This change reorganizes the front matter-related code so it is fully contained within the Bridgetown::FrontMatter module. That makes it easier to find the functionality and standardizes the way that Bridgetown refers to front matter. It also adds deprecated constant proxies for backwards-compatibility. It's unclear whether the moved modules were public API or not so the proxies make it so updating will not break anyone's site without warning.

This is already done by the include of the importer so isn't necessary at this point.

michaelherold · 2023-08-22T02:26:19Z

Rebased for the green checkmarks (and hit a flaky test).

jaredcwhite · 2024-02-17T16:17:27Z

@michaelherold Better late than never! This PR looks really good, so I'm going to go ahead and merge with the expectation to release in Bridgetown v2.0. If there were any more details you wanted to go over, just let me know in a follow-up issue/PR. Thanks!

…ers (#778)" This reverts commit b22ddfe.

jaredcwhite · 2024-03-16T19:54:13Z

@michaelherold don't worry, I didn't revert this for 2.0, just for the latest 1.3.3 point release (because I needed other stuff that was also in main). Still excited to get this in!

michaelherold commented Jul 3, 2023

View reviewed changes

michaelherold added 4 commits August 21, 2023 21:21

Remove unused constants

e1baa67

These were developed prior to FrontMatterImporter and are no longer used.

Remove unnecessary include

11d328d

This is already done by the include of the importer so isn't necessary at this point.

michaelherold force-pushed the front-matter-loaders branch from 2ca74e3 to 11d328d Compare August 22, 2023 02:22

jaredcwhite added this to the 1.4 milestone Aug 22, 2023

jaredcwhite removed this from the 1.4 milestone Oct 10, 2023

jaredcwhite merged commit b22ddfe into bridgetownrb:main Feb 17, 2024
1 of 4 checks passed

jclusso mentioned this pull request Feb 24, 2024

Issue with Resource API following #778 #851

Closed

jaredcwhite added a commit that referenced this pull request Mar 16, 2024

Revert "Refactor front matter handling and extract behavior into load…

8d20185

…ers (#778)" This reverts commit b22ddfe.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor front matter handling and extract behavior into loaders #778

Refactor front matter handling and extract behavior into loaders #778

michaelherold commented Jul 3, 2023

michaelherold left a comment

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

michaelherold Jul 3, 2023

jaredcwhite commented Jul 31, 2023

michaelherold commented Aug 4, 2023

michaelherold commented Aug 22, 2023 •

edited

Loading

jaredcwhite commented Feb 17, 2024

jaredcwhite commented Mar 16, 2024

		1. `HEADER` matches the opening line of the front matter
		2. `BLOCK` matches the contents of the front matter block with the first capturing group being the content and the regular expression consuming the ending delimiter

Refactor front matter handling and extract behavior into loaders #778

Refactor front matter handling and extract behavior into loaders #778

Conversation

michaelherold commented Jul 3, 2023

Summary

Context

michaelherold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredcwhite commented Jul 31, 2023

michaelherold commented Aug 4, 2023

michaelherold commented Aug 22, 2023 • edited Loading

jaredcwhite commented Feb 17, 2024

jaredcwhite commented Mar 16, 2024

michaelherold commented Aug 22, 2023 •

edited

Loading