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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `type` attribute to Document instances #7406

Merged
merged 2 commits into from May 16, 2019

Conversation

Projects
None yet
3 participants
@ashmaroli
Copy link
Member

commented Dec 6, 2018

  • This is a 馃檵 feature or enhancement.
  • I've added tests
  • The test suite passes locally

Summary

  • Maintains consistency between Jekyll::Page and Jekyll::StaticFile where instances the latter two classes respond to the :type method.
  • Optimizes Front Matter Defaults for Documents by not needing to check for both @doc.collection.label and @doc.collection.label.to_sym as is done currently

@ashmaroli ashmaroli added this to the 4.0 milestone Dec 6, 2018

@ashmaroli ashmaroli requested a review from jekyll/build Dec 6, 2018

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented May 16, 2019

Profiler summary

--- master branch https://travis-ci.org/jekyll/jekyll/jobs/532959217
+++ PR branch     https://travis-ci.org/jekyll/jekyll/jobs/533140377

- Total allocated: 532.65 MB (5301707 objects)
- Total retained:  18.34 MB (96237 objects)
+ Total allocated: 532.4 MB (5296981 objects)
+ Total retained:  18.33 MB (95977 objects)
Show resolved Hide resolved test/test_excerpt.rb
@@ -39,7 +41,7 @@ def initialize(path, relations = {})
end

data.default_proc = proc do |_, key|
site.frontmatter_defaults.find(relative_path, collection.label, key)
site.frontmatter_defaults.find(relative_path, type, key)

This comment has been minimized.

Copy link
@mattr-

mattr- May 16, 2019

Member

Was collection.label already a symbol? If not, is our replacing it with a symbol here safe?

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli May 16, 2019

Author Member

Yes, it is safe because, it is expected (from the comments):

# Finds a default value for a given setting, filtered by path and type
#
# path - the path (relative to the source) of the page,
# post or :draft the default is used in
# type - a symbol indicating whether a :page,
# a :post or a :draft calls this method
#
# Returns the default value or nil if none was found
def find(path, type, setting)
value = nil
old_scope = nil
matching_sets(path, type).each do |set|
if set["values"].key?(setting) && has_precedence?(old_scope, set["scope"])
value = set["values"][setting]
old_scope = set["scope"]
end
end
value
end

@mattr-

mattr- approved these changes May 16, 2019

@mattr-

This comment has been minimized.

Copy link
Member

commented May 16, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 49ffbbd into jekyll:master May 16, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details

jekyllbot added a commit that referenced this pull request May 16, 2019

@ashmaroli ashmaroli deleted the ashmaroli:document-type branch May 16, 2019

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