zoni
Frontmatter based filtering
This is attempt to implement #66
My version of this PR has so many file changes because I struggled to find a convenient and extensible way to pass the "yaml key" to the Postprocessor function. I settled on passing a reference of the Exporter after trying several other strategies (Box of a closure being one).
I ran into lifetime issues when passing the exporter by reference and the MarkdownEvents / Context by value, so I instead passed them as mutable references:
pub type Postprocessor = dyn Fn(&mut Context, &mut MarkdownEvents, &Exporter) -> PostprocessorResult + Send + Sync;
By doing this, I had to make quite a few minor changes. I added two tests for the yaml filtering and the cmd line arguments as well. All of the existing tests run successfully.
@zoni Let me know your thoughts! This is obviously a bit more of a structural change than you reference in #65 , so feel free to point me in a different direction.
Given the size of the changes, I'm going to need to find a moment where I have the time and energy to focus on a thorough review, but I did do a very quick pass over it. There will be a couple minor points I think will need to be adjusted a bit, but overall, I am very happy with what I saw so I'm definitely predisposed to merging this in 😄
I'm hoping I'll get a chance to leave some feedback for you this weekend. If you don't hear back from me within the next 5 days or so, please gently ping me with a nudge.
@zoni I really appreciate your guidance through this PR. I like the closure method better as well and like I said, I tried for an embarrassingly long time to get it to work. The crux of the issue was the move.
I will work on refactoring my PR and think more about https://github.com/zoni/obsidian-export/pull/67#discussion_r780667110!
I really appreciate your guidance through this PR. I like the closure method better as well and like I said, I tried for an embarrassingly long time to get it to work.
Happy to help! Rust is an unforgiving teacher. I still frequently run into these challenges myself as well, so you definitely shouldn't feel bad :slightly_smiling_face:
On a tangentially related note, I extracted the "pass context & markdown events as mutable references to postprocessors" changes into a separate PR (https://github.com/zoni/obsidian-export/pull/68). After refactoring all the postprocessors used in my (currently non-public) program which wraps obsidian-export's library (ultimately making up 90% of my publishing pipeline to nick.groenen.me) I have to say I like how much cleaner that looks.
Would you like me to hold off on merging #68 until this PR is wrapped up, or would you be happy rebasing on top of those changes?
I rebased to #68 and implemented the closure based on the example laid out in review-67/example-frontmatter-inclusion-with-closure
To address https://github.com/zoni/obsidian-export/pull/67#discussion_r780667110, I restructured the CLI as follows:
--frontmatter-export-filtering: a boolean that enables front-matter based export filtering
--frontmatter-filter-key: a string that sets the filter key (defaults to "export")
--frontmatter-filter-embeds: a flag to also filter embeds by frontmatter
Let me know your thoughts!
I've also started work on adding a contributor's guide to this project (see https://github.com/zoni/obsidian-export/pull/71). It's a bit late for you now, but the hints around rustfmt and pre-commit might still benefit you.
As a first-time contributor, if you happen to have any suggestions or other comments for it, I'd love to hear them.
As always @zoni, thanks for the comments and suggestions. I have been part of a few OS PRs and this has been by-far the most helpful, as well as introducing me to some really neat tools.
Regarding the contribution guidelines in #71, I think that they are well written and generally clear. The only small thing that I would add is that after pre-commit install, the user must run pre-commit run --all-files to trigger it without a commit.
The CI is still failing with the message: Option::unwrap() on a None value', tests/postprocessors_test.rs:185:14. From my testing, this is due to the fact that the tests parses the entire tests/testdata/input/postprocessors/ folder. That runs into my tests/testdata/input/postprocessors/yaml-filtering/_embed.md file, which doesn't have is_root_note in the front-matter. The embeded postprocessor here fails on the unwrap of None value.
My next commit will be a quick fix. If you want me to split it into a separate PR, I can do that as well!
Just a quick heads-up I definitely hope to look at this again soon, but this week's been really busy so I've not had the energy. Please give me a nudge if you haven't heard anything back from me in like a week's time :smiley:
Any more word on this PR? What else needs to be done? I'd really love this feature so I can publish my Obsidian vault to my Gatsby markdown blog
Just my two cents here: I created my own CLI tool based on this amazing library and implemented a very similar filtering as in this PR with post-processor rules. But one issue I'm facing is that all note attachments (such as images) are exported, even if all the referring notes are filtered out.
The easy part is collecting all the attachment references in the post-processor closure because it has access to the Markdown tags. The hard part is surfacing the list of attachment paths to be deleted for each skipped markdown file from the post-processing closure. I can't post-process the attachment files because those are simply copied over, and I can't delete them as a side-effect from the closure because of the random walk order.
With the risk of hijacking this thread, @zoni what are your thoughts on this issue? Do you think it's out of the scope for this filtering feature and all non-markdown files should be just exported? Do you have a suggestion for handling this within the constrains of this library maybe?
This has now been implemented through #163.
@ofalvai apologies, I'd missed/forgotten to respond to your question. If you're still struggling with this, I do have some possible suggestions, so if it's still relevant, let's open a separate discussion about that topic.