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

Jekyll messes up the kramdown hash, creating symbols and string. #6558

Closed
envygeeks opened this issue Nov 14, 2017 · 16 comments · Fixed by #7699
Closed

Jekyll messes up the kramdown hash, creating symbols and string. #6558

envygeeks opened this issue Nov 14, 2017 · 16 comments · Fixed by #7699
Assignees
Labels
accepted frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue

Comments

@envygeeks
Copy link
Contributor

envygeeks commented Nov 14, 2017

Blame: 143367c

screenshot-2017-11-14_13-43-00

@mattr-
Copy link
Member

mattr- commented Nov 14, 2017

Why is this important?

@mattr-
Copy link
Member

mattr- commented Nov 14, 2017

To further clarify, I understand that the duplication of keys and values is not ideal. Is that the only reason for this particular issue?

@envygeeks
Copy link
Contributor Author

envygeeks commented Nov 14, 2017

LOL, I'm out.

@DirtyF
Copy link
Member

DirtyF commented Nov 14, 2017

Seems legit, if the duplication is here for no reason and we can avoid, we will fix this.

/cc @jekyll/stability

@jekyllbot jekyllbot assigned parkr and ghost Nov 14, 2017
@pathawks
Copy link
Member

This was introduced in #6247 to fix #5980. In short, kramdown wants option keys to be symbols, but we get them from YAML as strings.

After we have merged the config hash with our defaults hash, we go through and symbolize all the string keys.

def make_accessible(hash = @config)
hash.keys.each do |key|
hash[key.to_sym] = hash[key]
make_accessible(hash[key]) if hash[key].is_a?(Hash)
end
end

This results in items that have been added with a string key being duplicated.

I don't see any advantage in performing deduplication after this step, but perhaps I am missing something.

We might be able to use symbols instead of keys for our defaults:

"kramdown" => {
"auto_ids" => true,
"toc_levels" => "1..6",
"entity_output" => "as_char",
"smart_quotes" => "lsquo,rsquo,ldquo,rdquo",
"input" => "GFM",
"hard_wrap" => false,
"footnote_nr" => 1,

Then we would need to call make_accessible on the overrides before merging, I guess. The downside here is that none of our other defaults seem to use symbol keys, so that might be confusing.

Is the motivation here purely theoretical, or is there something real to be gained? @DirtyF?

@DirtyF
Copy link
Member

DirtyF commented Nov 15, 2017

If Kramdown wants option keys, I don't see why we don't set Kramdown defaults as option keys from the start. I don't know about the gain, maybe do plugins developers need to parse the kramdown options?

@pathawks
Copy link
Member

@DirtyF The problem is merging in YAML, since those will all have string keys. Also, taking a look at the defaults everything else has a string key. If I didn't know anything about this issue, and saw that some options were strings and some options were symbols, I would be confused.

We can't be sure that plugin developers don't rely on the current behavior of accessing via string keys rather than symbols, so any change would have to bump the major version number I suppose.

@ashmaroli
Copy link
Member

Can we not symbolize these keys just before being passed onto Kramdown instead of storing them as symbols?

@pathawks
Copy link
Member

We do symbolize them just before passing them to kramdown; we just don’t remove the string keys.

@ashmaroli
Copy link
Member

ashmaroli commented Nov 15, 2017

I meant something like..

hash = @config.dup
Kramdown::Document.new(content, make_accessible(hash)).to_html

@pathawks
Copy link
Member

@ashmaroli I don’t understand what that accomplishes, but then I really don’t fully understand the problem here anyway, so I defer.

@jekyllbot jekyllbot added the has-pull-request Somebody suggested a solution to fix this issue label Nov 15, 2017
@ashmaroli ashmaroli removed the has-pull-request Somebody suggested a solution to fix this issue label Nov 21, 2017
@jekyllbot

This comment has been minimized.

@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Jan 21, 2018
@parkr
Copy link
Member

parkr commented Jan 21, 2018

We didn’t fix this yet? I see the PR referenced above was closed. It would be nice to normalize this to whatever kramdown expects. I’d personally be fine with a solution that simply creates a new hash, copies only kramdown settings into the new hash as symbols, and passes that to kramdown (I.e. don’t change the site.config at all). Thoughts?

@jekyllbot jekyllbot removed the stale Nobody stepped up to work on this issue. label Jan 21, 2018
@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Apr 30, 2018
@jekyllbot

This comment has been minimized.

@pathawks
Copy link
Member

For Jekyll 4.0, maybe we symbolize all the things and only keep the new, symbolized hash.

@jekyllbot jekyllbot removed the stale Nobody stepped up to work on this issue. label Apr 30, 2018
@jekyllbot
Copy link
Contributor

This issue has been automatically marked as stale because it has not been commented on for at least two months.

The resources of the Jekyll team are limited, and so we are asking for your help.

If this is a bug and you can still reproduce this error on the 3.3-stable or master branch, please reply with all of the information you have about it in order to keep the issue open.

If this is a feature request, please consider building it first as a plugin. Jekyll 3 introduced hooks which provide convenient access points throughout the Jekyll build pipeline whereby most needs can be fulfilled. If this is something that cannot be built as a plugin, then please provide more information about why in order to keep this issue open.

This issue will automatically be closed in two months if no further activity occurs. Thank you for all your contributions.

@jekyllbot jekyllbot added the stale Nobody stepped up to work on this issue. label Jul 1, 2018
@ashmaroli ashmaroli reopened this Jun 6, 2019
@ashmaroli ashmaroli added has-pull-request Somebody suggested a solution to fix this issue and removed help-wanted stale Nobody stepped up to work on this issue. labels Jun 6, 2019
@jekyll jekyll locked and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted frozen-due-to-age has-pull-request Somebody suggested a solution to fix this issue
Projects
None yet
7 participants