-
Notifications
You must be signed in to change notification settings - Fork 176
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
Support for custom front matter keys in notebook-based posts #14
Support for custom front matter keys in notebook-based posts #14
Conversation
e8a6a00
to
71043b9
Compare
@xnutsive I can perhaps offer some suggestions or clarification ( although I am still learning things) If you look at the default template used by nbdev jekyll-md.tpl you will see that classes are injected to contain code and markdown blocks, which are subsequently used by this repo's custom CSS file main.scss. It seems that is missing from your template, and perhaps may break the styling? ( What happened when you ran a notebook through your template and tested it end-to-end? )
In my experiments, files must be *.md in My humble suggestion is we take your proposal, test it outside this repo end-to-end and use the new flexibility to include additional useful features such as links to collab etc, and open a PR at that point? I'm happy to help with this if appropriate. Most importantly, we should wait for Jeremy to comment to see how to proceed etc. (It might be the case that more discussion could be useful before writing any more code). |
@hamelsmu thank you for reviewing and commenting this! I agree, it's important to get this right and not break things, so testing outside of the repo before this is merged in sounds like a good idea. I'm writing a couple articles with this setup and I'll link them once they're published, should be today and tomorrow, but the more tests the better.
As far as I understand, The template committed is jekyll.tpl with rewritten front matter. I've also dropped the
I agree, since markdown parses mostly allow html, it makes sense to convert to html, that doesn't break anything. I'll post an end to end example later tonight ;-) |
You are right 🙇 I was mistaken about this |
Here's an example! https://respawn.io/2020/Hello-world/ Looks like the nb cells, python code, and charts work fine. Surprised by how fast this converts and deploys in the gh action. @hamelsmu, thank you so much for putting this together. 👏 |
Some questions (feel free to ignore)
I'm not sure offhand how to automatically inject the links automatically, perhaps could inject this into your template with Also, I do not deserve credit for fast_template, I only made some small tweaks that automate what is already here before, real thanks goes to Jeremy |
One more thing I should probably mention is I don't really make decisions on what features should / should not be included here. Which is why it could be a good idea to talk to Jeremy first. ( I don't want to give you the wrong impression that I have maintainer rights etc ) |
I understand that, and I appreciate you putting your time and energy in implementing the notebook conversion gh actions, and in reviewing and discussing my tweaks!
Why, yes, we could do a snippet in Not sure if |
I'm not as familiar with gh pages/actions as Hamel, so I'm not sure I have a huge amount to add. In general, I'm very happy to add features that don't significantly increase the complexity of the project, and (most importantly) don't in any way impact things for people that don't need those features. Badges for colab and (perhaps) nbviewer and mybinder I think would be pretty useful, if they can be added within the above constraints. |
I believe in this approach, too. To ensure this addition complies with the rule of thumb:
Note: ideally, I want to test this with at least one more post myself before we merge this, just to be sure that no metadata is broken.
Perfect! Thanks for the heads up, I'll take a look and try to implement it and add them to this PR. I'll comment when I'm done, likely by Saturday morning PST. |
@xnutsive I just want to comment that your new template is incredibly helpful! I am already using it in another project and it works great for passing extra front matter, thank you so much for adding this. Incase it is helpful, this repo comes pre-loaded with Primer.css so for the "View on GitHub Button" can be accomplished like this: <div class="clearfix">
<a class="btn btn-sm btn-with-count" href="#url" role="button">
{% octicon mark-github %}
<span>View On GitHub</span>
</a>
</div> Hope that helps |
Hi @xnutsive I've used this template enstively over the past couple of days and have some questions.
<div class="container" id="notebook-container">
{{ "{% raw %}" }}
{{ super() }}
{{ "{% endraw %}" }}
</div> |
Seeing how quickly fast_template evolves, I'll try to wrap things up here quicker. I've updated the initial message with a todo list for myself. @hamelsmu, thanks for your comments, for trying it out and testing it!
And number 3 being add the github and colab buttons as {{ includes }}. Hope to finish this tomorrow, I'm constrained on time this week a bit. |
Thanks this is very helpful. I'm on a book deadline until Feb 10, so have limited time to look at things, but if you have something you think is mergeble before then, please let me know.
|
71043b9
to
ad15f0b
Compare
@hamelsmu, heads up! I've addressed the whitespace problem, and added colab and github button snippets. At this point, I think if someone really wants to customize them further (or just use links, not buttons) they'd be able to just look in the code. As for the escaping problem, it's super interesting. So we have two templating engines here:
I've tried to debug it first, and found out that:
But you referred to a problem with a code cell, which makes me think maybe I still miss something. I can just throw the raw wrapper tag in the template, but I'm not that sure we should do that by default — maybe give users the flexibility, but document this behavior instead. What do you think if we merge this PR, and I create another issue for raw / endraw with markdown and take on investigating and fixing it? |
@xnutsive 👍 I agree with you it totally makes sense to tackle the escaping issue in a different PR. This LGTM so far 🙇 except for one minor comment below where might be good to double-check Once you/me open an issue re: escaping I'll drop a sample notebook that explodes in there for us to debug, but for now we can take that conversation outside this PR. Thanks so much for working on this |
@@ -9,6 +9,7 @@ | |||
title: Edit _config.yml to set your title! | |||
description: This is where the description of your site will go. You should change it by editing the _config.yml file. It can be as long as you like! Happy blogging... ❤ | |||
github_username: jph00 | |||
github_repo: blog # used for puslishing links back to your repo in the blog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the addition of this break anything? Lets say if the person who uses this template doesn't name their repo blog
? I think that may conflict with the setup instructions?
The instructions in the blog post tell people to setup the blog so it will run off off root
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a new setting, the only thing that would be broken as you pointed out is the github and colab links, unless users actually go and update their _config.yml
.
If they use a different repo name, and then use {% github_notebook_link %}
, the page would build, but the link would be broken.
I think once someone starts investigating, they'll find the setting in a few minutes (since the snippet users {{site.github_repo}}
right after {{site.github_user}}
, and it's a common Jekyll config lookup pattern. But ideally, this can be documented, and thus it'll be even easier for users.
One alternative is to do github_repo : %CHANGE_ME_IN_CONFIG_YML%
— that would be broken as well, but that would point people to where they should fix it. I think about.md
does this, and description:
setting by default says something in the nature of This is where the description text is used, change me in config.yml
.
Should I change it to something descriptive like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see. In this case you might not need the the github_repo
variable at all. This template comes pre-loaded with the github-metadata plugin, which allows you to reference the full path as
{{ site.url }}{{ site.baseurl }}
For example, I am doing this for word docs
@hamelsmu @xnutsive sent you both invites to https://github.com/fastai/fastpages . Will close this. I'm on a book deadline until after Feb 10 so feel free to do whatever you like in that repo in the meantime, and I'll try to catch up later! I suggest in fastpages we use @hamelsmu could you do a PR to remove all the stuff for _word/_notebook etc in this repo? |
@@ -0,0 +1,4 @@ | |||
<a class="btn btn-sm text-gray-dark" href="https://github.com/{{site.github_username}}/{{site.github_repo}}/tree/{{page.branch | default: "master" }}/{{page.nb_path}}" role="button"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a class="btn btn-sm text-gray-dark" href="https://github.com/{{site.github_username}}/{{site.github_repo}}/tree/{{page.branch | default: "master" }}/{{page.nb_path}}" role="button"> | |
<a class="btn btn-sm text-gray-dark" href="{{ site.url }}{{ site.baseurl }}/tree/{{page.branch | default: "master" }}/{{page.nb_path}}" role="button"> |
I believe this will work per my earlier comment re: the special plugin already built in
@@ -0,0 +1,3 @@ | |||
<a class="btn btn-sm text-gray-dark" href="https://colab.research.google.com/github/{{site.github_username}}/{{site.github_repo}}/blob/{{page.branch | default: "master"}}/{{page.nb_path}}" role="button"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a class="btn btn-sm text-gray-dark" href="https://colab.research.google.com/github/{{site.github_username}}/{{site.github_repo}}/blob/{{page.branch | default: "master"}}/{{page.nb_path}}" role="button"> | |
<a class="btn btn-sm text-gray-dark" href="https://colab.research.google.com/github/{{ site.github.repository_nwo }}/blob/{{page.branch | default: "master"}}/{{page.nb_path}}" role="button"> |
See my comment regarding the plugin, might want to test this works, but i got {{ site.github.repository_nwo }
from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a some suggestions on how you can utilize the plugin in the path construction
@jph00 Sure I'll send the PR now |
@xnutsive I'm working on the new repo now. I'm going to put the base information in there, and will comment on this thread to get this PR in there! Or if you prefer I can copy-paste your code, however I would prefer your name shows up on the contributions history. I'll comment again on this thread when I'm done with setup (could take me 2-3 days to complete this step. ) . I'm going to try and set it up a bit differently based on some problems I observed with this template, and add tests etc to facilitate PRs. Looking forward to working with you on the new repo 🙇 |
Noice! I agree that keeping As for this PR — I don't mind to wait for a bit and then port it to the new repo at all. No rush. It's also OK if you want to quickly copy and paste the code as a boilerplate / initial version for the repo, and you can always credit me in the readme if you see fit. As for other contributions, and plan of work, if there's a google doc already — could you please invite me as well, using nate@respawn.io? Alternatively, if you think it's a good idea, we could start a discussion or a series of topics on forums.fast.ai, in the nbdev section? I'm very excited to have the opportunity to learn from you, and to build together — you can count on something in the nature of 4-8 hours a week. Feel free to offload any features / chores / issues or bugs you'd want me to focus on. |
Thanks @xnutsive look forward to working with you too, its great to have someone in the community that is excited about this.
Would rather have you do a PR if possible b/c I'm lazy and don't want to maintain a README of contributors over time 😄 (I'm still working on building the structure of the repo, I'm currently working on it in a fork )
Its a good idea to use the fast ai forums, I'll start a thread there and tag you today. Sorry I was supposed to do this earlier and forgot.
I'm here to learn, too. GitHub as allowed me to work on this task for a part of my workday every day :) so hopefully I will be able to help/resolve issues |
I'm here to learn, too. GitHub as allowed me to work on this task for a part of my workday every day :) so hopefully I will be able to help/resolve issues
Wow that's awesome news!
|
@xnutsive ok I believe that the new repo is ready for a PR! It took me some time (longer than expected) to setup good CI tools there to try to make testing / maintenance easier. Your PR will be a good chance to test out automation there. One request can you put your new template in |
@xnutsive I'm working on integrating this into I will mention you in an acknowledgment section of the README, as well as tag you for review on the PR. Thank you so much again for this work, it is very helpful |
Hi there!
Thank you for stepping in for me, and for nudging me earlier. I'm sorry to miss out these two weeks, I'm almost ready to get back into work mode — will catch up on the current scope on the weekend and write some code then.
…On Thu, Feb 13, 2020 at 12:40 PM, Hamel Husain < ***@***.*** > wrote:
@ xnutsive ( https://github.com/xnutsive ) I'm working on integrating this
into fastpages now, as it will enable lots of cool things.
I will mention you in an acknowledgment section of the README, as well as
tag you for review on the PR. Thank you so much again for this work, it is
very helpful
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (
#14?email_source=notifications&email_token=AAAKU4OCX7RCHBDSXCAUTADRCWV5TA5CNFSM4KMYJKE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELWRABA#issuecomment-585961476
) , or unsubscribe (
https://github.com/notifications/unsubscribe-auth/AAAKU4O6IGCGZUX3D3AZYXDRCWV5TANCNFSM4KMYJKEQ
).
|
@xnutsive please don't worry about it. I have lots of time to work on this, and I am here to help out. I am making the changes to |
We discussed this with @hamelsmu on Twitter yesterday — this is a bit more complex than I thought.
Context
When we process a notebook post with nbdev, we use the default nbdev Jinja template to convert it to html (and misteriously use .md extension for the resulting file, but that's another issue). The default template doesn't recognize any custom keys in the front matter, it just uses the keys utilized by nbdev for lib / docs development:
title
,summary
,toc
, etc.Hamel writes what makes perfect sense in the docs: you should be able to use any keys you want and pass those to posts, and then use those in your post templates in Jekyll (which are Liquid templates. Similar, but not the same as Jinja).
Problem is, this currently doesn't work.
Proposal
I've implemented the custom keys support in this PR and I'll use something like that in my blog (fork). The basic idea is to:
to
html`.nb2post.py
The template would iterate through everything that's passed in it as
resources
and output all the custom keys.One caveat is that nbdev passes a bunch of internals into the template, and we don't want to put that in the template. I filter those out by default, and if you need them for any reason, you can use them. For example, I left
nb_path
exposed in case you want to put a link to the notebook on github in the rendered post.What do you think?
UPD few things to finish this PR and get it merged as discussed with @jph00 and @hamelsmu below: