Skip to content

Add --linked-attachments-only option#316

Open
jajaperson wants to merge 3 commits into
zoni:mainfrom
jajaperson:main
Open

Add --linked-attachments-only option#316
jajaperson wants to merge 3 commits into
zoni:mainfrom
jajaperson:main

Conversation

@jajaperson

@jajaperson jajaperson commented Dec 7, 2024

Copy link
Copy Markdown

Adds support for #217 via a --linked-attachments-only flag. When enabled, any non-markdown files will only be included if they are linked or embedded within a markdown file.

@jajaperson

Copy link
Copy Markdown
Author

I've just worked out that this approach doesn't work with the tag filtering options, I will try fix this now.

Make `linked_attachments_only` option compatible with tag filtering,
i.e. only occur after post-processing.

@zoni zoni left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you so much for working on this. I've yet to do a more thorough review of the code, but at a glance, I think this looks alright, so I'm favorable towards accepting it 😄

However, I would like to see some tests being included to validate the intended behavior (including the interaction with tag filtering options) and guard against future regressions as outlined in Contributing > Tests.

Would you mind wiring up some test cases? I'll be happy to provide some additional pointers if needed.

@jajaperson

Copy link
Copy Markdown
Author

Sure, I'll be able to write some tests in a few days. By the way, the way it's currently implemented, any attachments detected during markdown parsing are added to a HashSet. Once parsing is complete for that file, the export_note method is called for each of its attachments. So potentially, the same attachment is getting exported more than once, if it is linked in multiple markdown files. Could this, to your knowledge, cause any issues? Ideally there would be a centralized HashSet collated over all markdown files which is then iterated over at the end, but I didn't come up with an easy solution for this with the way the Exporter is currently setup with parallelism.

@jajaperson jajaperson requested a review from zoni December 14, 2024 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants