Frontmatter based filtering

[mschrader15]

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.

[zoni]

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.

[mschrader15]

Thanks @zoni, sounds good!

[zoni]

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 did a bit of experimentation and came up with an approach that works without requiring any changes to the current Postprocessor type. Taking the relevant code from this PR (I ignored tests), I created an annotated example for you on the branch review-67/example-frontmatter-inclusion-with-closure. The relevant commit is 00e0f70.

Aside from avoiding the need to change the Postprocessor signature, this avoids any changes to the Exporter struct as well, which is another thing I'd like to avoid wherever possible. This way the "yaml inclusion" postprocessor is completely independent from the core Exporter.

If you could refactor your PR to use this approach, I think we'll be very close to having this in a shape that is ready to be merged! 🙌

With all this being said, I don't think your efforts of passing Context and MarkdownEvents as mutable references to postprocessors were entirely wasted. It does make the API a bit more ergonomic looking, requiring only the return of a PostprocessorResult. I'm going to think on it for a bit, but I may keep that change (applied through a separate PR) 🙂

[zoni]

I haven't come up with a better flag yet, but I'm not 100% happy with the naming here. My main concerns comes from the fact this does two things (it both sets the key to look for, and also toggles the behavior of only including notes with that key set) however the naming implies it only does the former.

Perhaps those two things should in fact be two separate arguments?

[mschrader15]

@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 #67 (comment)!

[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.

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 🙂

On a tangentially related note, I extracted the "pass context & markdown events as mutable references to postprocessors" changes into a separate PR (#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?

[mschrader15]

I rebased to #68 and implemented the closure based on the example laid out in review-67/example-frontmatter-inclusion-with-closure

To address #67 (comment), 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!

[zoni]

@mschrader15 we're so close! Just a few final changes needed 😅

1. I think your last merge went wrong and introduced some conflicts:

   Auto-merging src/postprocessors.rs
   CONFLICT (content): Merge conflict in src/postprocessors.rs
   Auto-merging tests/postprocessors_test.rs
   error: could not apply 79ab601... Chg: Pass context and events as mutable references to postprocessors
   hint: Resolve all conflicts manually, mark them as resolved with
   hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
   hint: You can instead skip this commit: run "git rebase --skip".
   hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
   Recorded preimage for 'src/postprocessors.rs'
   Could not apply 79ab601... Chg: Pass context and events as mutable references to postprocessors
   

2. Not all files are formatted according to rustfmt conventions, which will get rejected by CI. rustfmt **/*.rs should fix that for you.
3. I left a few changes on comments, docs, etc. You should be able to commit these suggestions as-is, but feel free to extend on them.

[zoni]

I've also started work on adding a contributor's guide to this project (see #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.

[mschrader15]

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!

[zoni]

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 😃

[mschrader15]

No worries!

[dreamworld]

Any progress on this PR? I'm expecting for this feature

[Sewdohe]

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

[ofalvai]

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?

[zoni]

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.