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

Allow user values to override drop-determined values #110

Merged
merged 7 commits into from Sep 7, 2017

Conversation

benbalter
Copy link
Contributor

Via #108 (comment), it appears user-supplied site.github values are being overwritten by drop-supplied values.

It appears in 54942a4#diff-165b64b2f6bce98447b6c550f3205ad9L26, the order of the arguements passed to Jekyll::Utils.deep_merge_hashes were reversed since the right overrides the left value.

This PR adds failing tests via a7d2b4b to ensure site.github is set to the expected value given different user-supplied inputs for site.github with d36d1af reversing the order of the arguments.

/cc @DirtyF

@benbalter benbalter self-assigned this Aug 29, 2017
@benbalter benbalter requested a review from parkr August 29, 2017 22:22
@benbalter benbalter mentioned this pull request Aug 29, 2017
@DirtyF
Copy link
Member

DirtyF commented Aug 30, 2017

@benbalter FWIW I just did a test with this branch and it does still output the drop value from GitHub, not the one from the local config file, even with the switch of the arguments of deep_merge_hashes. 🤔

Here's a link to the build preview, you can see that the link "Editez le contenu" still points to gh-pages.

Happy to help debug you this.

@benbalter
Copy link
Contributor Author

benbalter commented Aug 30, 2017

@DirtyF backported (forward-ported?) jekyll/jekyll#6338 via 7a97ed0. Let me know if things work as expected now?

@@ -30,7 +30,7 @@ def github_namespace
when nil
drop
when Hash
Jekyll::Utils.deep_merge_hashes(site.config["github"], drop)
Jekyll::Utils.deep_merge_hashes(drop, site.config["github"])
Copy link
Member

Choose a reason for hiding this comment

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

Flipping this may have some unintended consequences (because people expect the opposite) so we should be prepared for upset users.

@DirtyF
Copy link
Member

DirtyF commented Aug 30, 2017

@benbalter the issue is not resolved in my case{{ site.github.source.branch }} still output gh-pages 😞

Test: DirtyF/frank.taillandier.me@85987a8

@benbalter
Copy link
Contributor Author

benbalter commented Sep 5, 2017

@DirtyF see jekyll/jekyll#6338 (comment). Mind giving it one more try? (I tested locally with a test site and it worked as expected).

@benbalter
Copy link
Contributor Author

Test: DirtyF/frank.taillandier.me@85987a8

Tested locally and go the following (expected) output:

  <footer>
    <p>Une correction à apporter ? <a href="https://github.com//edit/master/_posts/2017-08-27-securite-psychologique.md">Éditez cet article</a>.</p>
  </footer>

@benbalter
Copy link
Contributor Author

@jekyllbot: merge +minor.

@jekyllbot jekyllbot merged commit 859f86e into master Sep 7, 2017
@jekyllbot jekyllbot deleted the user-overrides branch September 7, 2017 14:19
jekyllbot added a commit that referenced this pull request Sep 7, 2017
@jekyll jekyll locked and limited conversation to collaborators Apr 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants