search

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

unncessary breaking change with --markdown flag #321

Closed phanirithvij closed 1 year ago

phanirithvij commented 1 year ago

tldr node-js client still has the -m/--markdown flag still instead of the -r/--raw flag. https://github.com/tldr-pages/tldr-node-client/blob/8de0d9d60f71ac4c6a30c4ad1f5e59cd8210e94c/bin/tldr#L24

navi uses --markdown https://github.com/denisidoro/navi/blob/6f316ec7eb2f1f56a7609048e67860eb84c1aacf/src/clients/tldr.rs#L54

And it recommends installing rust client for tldr which I assume is this (tealdeer) https://github.com/denisidoro/navi/blob/6f316ec7eb2f1f56a7609048e67860eb84c1aacf/src/clients/tldr.rs#L9.

Now navi has to remove the rust recommendation and recommend python client in its place. Which is a bummer as python/node needs to be installed in the system, and rust version being a single binary is clearly superior.

There is no need to break this, https://github.com/dbrgn/tealdeer/pull/290 and the other flags too. If possible to maintain backward compatability please do!

Someone else had the same issue.

niklasmohrin commented 1 year ago

Sorry for the long wait. Well, there is no need to remove deprecated options, but one would want to eventually get rid of them to reduce code complexity, compile time, etc. As for why the markdown flag was renamed to raw: We at some point felt that raw is a better name, because the output is not markdown that is ready to be displayed as is, but still has the {{ markers in it that tealdeer highlights as user input (the commit that made this change is 8833b6b40179eef6161035d10d7e218694facca1). I am not sure how to proceed here, best case would be if the tldr project could decide a name for the desired behavior. I think I would oppose re-introducing the alias unless there is a good reason to do so.

phanirithvij commented 1 year ago

I understand the need to call it raw instead of markdown but as I said it breaks navi and is not compatible with all the other clients. It could have been left untouched as an extra cli flag wont affect compilation time, code complexity.

arashm commented 1 year ago

True. It renders tealdeer useless with thirdparty clients. Just realized it doesn't work with navi.

niklasmohrin commented 1 year ago

It renders tealdeer useless with thirdparty clients.

Not true, navi just has to stop making the wrong assumptions. The --markdown flag is not part of the tldr client specification and just happens to be a feature that a number of clients shared. Tealdeer decided to rename the flag, the previous name worked with a deprecation warning for over a year and no one complained. To be honest, I feel that if navi doesn't have it's dependencies under control, it's not our issue. Have you considered opening an issue with navi?

phanirithvij commented 1 year ago

Maybe this should then become a part of the tldr client specification if a number of clients share this?

the previous name worked with a deprecation warning for over a year and no one complained

I have only recently switched to tealdeer and thus navi broke only after I switched to tealdeer and thus I am a little late for the complaint box, I would've raised an issue had I discovered tealdeer early and saw the deprecation notice.

To be honest, I feel that if navi doesn't have it's dependencies under control, it's not our issue

Tealdeer is not a navi dependency. Tldr is the navi dependency which I intend to replace with a better version which is tealdeer. But alas tealdeer is not compatible with navi because the markdown flag was deprecated for no reason whereas it once was compatible.

And tealdeer names/declares itself to be tldr in the PATH env variable so I assume the intent is to act as a tldr client and being as compatible with the python implementation as much as possible. And the python version has the m flag so I don't get why the markdown flag isn't in the client specification other than that most users never used it so much for it to make or break their workflow.

To be honest there is no proper justification for this breaking change. And it is not navi's responsibility to check if the markdown flag fails and fallback to raw flag which only tealdeer uses among the tldr client implementations, this is convoluted.

And I would like @dbrgn's input on this issue too as he was the one who made the change.

I opened the issue here first in hopes it can be rectified at the source instead of going straight to implementing a workaround. I'll open an issue there as well once this issue reaches some kind of conclusion.

phanirithvij commented 1 year ago

I am not sure how to proceed here, best case would be if the tldr project could decide a name for the desired behavior

@niklasmohrin I am saying the same thing, I must've missed this part of your first response.

dbrgn commented 1 year ago

Maybe this should then become a part of the tldr client specification if a number of clients share this?

The client specification is here: https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md

Feel free to open an issue in that repository to suggest standardization! If a flag is standardized, then we'll implement it with high probability.

And tealdeer names/declares itself to be tldr in the PATH env variable so I assume the intent is to act as a tldr client and being as compatible with the python implementation as much as possible.

No, the goal is to act as a tldr client and being compatible with the client specification.

To be honest there is no proper justification for this breaking change. And it is not navi's responsibility to check if the markdown flag fails and fallback to raw flag which only tealdeer uses among the tldr client implementations, this is convoluted.

It's not tealdeer's responsibility to conform to navi's expectations either 😉 If navi wants a stable API that is compatible with all standards-conforming tldr clients, then the API should be standardized through the client specification linked above. If navi only works with the Python client, then they should document that (and users should follow that).

In my opinion, the best approach for navi would be to simply pull down the cache directly, without relying on other clients being installed. It's just a few lines of code, and it prevents incompatibilities like the one you have discovered.

To be honest there is no proper justification for this breaking change.

There is: As @niklasmohrin explained, we think it's a better-fitting name. We develop this project in our free time, and do what we think is best for the long-term goals of the project.

dbrgn commented 1 year ago

As a last comment: Every change has the potential to break something. See https://xkcd.com/1172/. If we keep compatibility with all historic options, then we end up with an ossified, hard-to-maintain piece of code.

Also, I kinda forgot to mention this: If the previous version of tealdeer suited your needs, you could use that older version? It still works fine. Nobody forces you to upgrade to newer versions. Since Rust projects can be built into a single binary, you can simply throw that into your /usr/local/bin and it will keep working.

phanirithvij commented 1 year ago

Ok I'll open a new issue over at tldr linking to this issue and raise a proposal for markdown/m flag and also suggest renaming it to raw, I understand open source and the effort that goes into it.

And also request navi to not rely on the markdown flag and reimplement things like you said.

And I will consider downgrading, thanks for the response.