dbrgn / tealdeer

A very fast implementation of tldr in Rust.
https://dbrgn.github.io/tealdeer/
Apache License 2.0
4.09k stars 124 forks source link

Custom Pages With `.md` Extension #320

Closed zedseven closed 6 months ago

zedseven commented 1 year ago

I really like the custom pages functionality, but it's not clear to me why the custom pages use a .page extension when the stock pages use .md.

Is the reason simply that the patch files use .patch?

The reason I'm interested in this is that I'd like to store my custom pages in an Obsidian vault as notes. This way, I can maintain command usage information in my regular notes app and have it accessible from the command line with tealdeer. Unfortunately, Obsidian expects the files to have a .md extension, which complicates things.

It would be fantastic to be able to define a custom extension or use .md for the custom pages. I'm not sure about how the patch files would have to be handled, however. For me, I don't see myself using patches, but it would be great to come to a solution that works for them too.

I'm happy to work on a PR for this, but before doing anything, I want to make sure I understand the decisions that were made, and what the best design would be.

zedseven commented 1 year ago

Perhaps a possible solution would be to support having 2 extensions at once?

Something like:

In my opinion, this also communicates the contents of the files more clearly. e.g. zstd.patch.md is a Markdown file to be used as a patch for the zstd command page.

It also allows for proper Markdown syntax highlighting for these files, without configuring an IDE to treat .page and .patch as Markdown.

niklasmohrin commented 1 year ago

yeah, we can probably be a bit nicer by using .page.md and .patch.md (or even just .md for custom pages). The problem is of course that everyone that already has custom pages with the current naming scheme would need to rename all their files. We could have a transition period where both naming schemes are accepted though and issue a deprecation warning when the old extension is found. What do you think? @dbrgn

niklasmohrin commented 1 year ago

Oh and sorry for the long wait :)

zedseven commented 1 year ago

No worries, I understand how it goes. :)

I don't know if you saw but I actually already submitted a PR with this functionality (#322), and I've been using a custom build with it for a while now.

I think to make the distinction clear, custom pages should probably be .page.md instead of just .md.

niklasmohrin commented 1 year ago

Yup, saw it, just wanted to keep discussion about the feature in one thread and move to PR for implementation details then. I am currently neutral between .page.md and .md

niklasmohrin commented 1 year ago

@dbrgn What do you think?

dbrgn commented 1 year ago

I really like the custom pages functionality, but it's not clear to me why the custom pages use a .page extension when the stock pages use .md.

Is the reason simply that the patch files use .patch?

Yeah, that was / is the case.

From a feature perspective, using .patch.md and .page.md wouldn't be bad, but from a maintenance perspective I'd rather not change anything. More code and more supported variants means more file system lookups (impacting all current users of tealdeer) and more complexity. On the other hand, the use case with Obsidian seems quite uncommon.

If you agree, @niklasmohrin, I'd close this feature request as "not planned". (Thanks @zedseven for the suggestion though.)

zedseven commented 1 year ago

@dbrgn, what I'm proposing is to replace the existing file extensions - you can see my proposed changes in #322.

Obviously it would be a breaking change, but it wouldn't add maintenance overhead and I think it makes sense to use .page.md and .patch.md.

The purpose of the file extension is to communicate the type of contents, and in my opinion, .page and .patch don't adequately communicate that the contents are Markdown.

I agree that my Obsidian use case is rather niche, but this is a change I'd make on principle alone.

The current situation makes working with these custom files more difficult for all editors. For example, it requires you to tell your editor that .page is Markdown, and in the case of .patch, that extension is already used for Git patches. That means users either have to override file type detection for file extensions that already have other meanings, or they have to put up with incorrect syntax highlighting for these files.

By changing the extensions to .page.md and .patch.md, none of this file type mess is needed and tealdeer can still understand which ones are patches vs. full new pages.

niklasmohrin commented 1 year ago

I must say I do prefer the new scheme over our current naming. I thought that we could maybe have a transition period where a deprecation warning is shown if the old naming is found. This would mean having a release some time soon and then another in, lets say, a year

One problem I see is that we would probably have to check if any files with the old format exist regardless of which page is queried because while it is very likely that users with custom pages / patches run tealdeer during the next year, but not so likely that they will also query exactly those pages they have modified. It will probably not be that expensive in the end, because the directories for custom pages / patches are not too filled.

Bottom line, I would vote deprecation and move to new scheme

niklasmohrin commented 1 year ago

@dbrgn Should we conclude something? I haven't changed my mind since my last comment, what do you think?

dbrgn commented 7 months ago

Hmm, I'm still a bit unsure whether it's worth it to change the naming. However, I do agree that a .md extension would have been the better choice.

One problem I see is that we would probably have to check if any files with the old format exist regardless of which page is queried because while it is very likely that users with custom pages / patches run tealdeer during the next year, but not so likely that they will also query exactly those pages they have modified.

We could add a transitional check in the source code, that checks whether any .patch or .page files exist in the custom pages dir, and then print a warning. The check could be removed after 1 year or so.

To avoid the (tiny) performance hit on page lookup, we could only do the check on update. However, people without auto-update enabled might run the update only rarely, and might not notice the change at all.

These things are what I meant with maintenance overhead. Additionally, there will be people that miss the change, and that may end up asking for help why their custom pages don't work anymore. But the feature hasn't existed for that long yet, and if we have a temporary warning, I think it should be fine.

TL;DR: Let's do it, if there's a warning for existing users of this feature.

zedseven commented 7 months ago

I've updated the PR with the requested changes - if there's anything else that needs to be done, please let me know.

niklasmohrin commented 6 months ago

I think if we make sure that the migration warning was in, let's say, Debian for a year, I am pretty sure that we will reach everyone.

If we introduce more migrations in the future, we could also think about adding something like tldr --check-health.