tldr-node-client
tldr-node-client copied to clipboard
tldr-pages
change `-o/--os` to `-p/--platform`
Closing→reopening the PR can frequently fix the checks, not the case here. I think you may have broken it, let me see. I'll also re-run the jobs.
The tests should pass now. FYI @marchersimon when you change the flags in the code (here) you also need to update references to that flag, e.g. program.os (here) corresponds directly to the string values passed to program.option, so it needs to be changed to program.platform. All of that and more I've done in the past two commits 👍🏻
Oh, sorry, I just blindly replaced -o and --os without checking if they occur somewhere else. Thanks for the fix
Closing→reopening the PR can frequently fix the checks, not the case here. I think you may have broken it, let me see. I'll also re-run the jobs.
OT Please don't do that because it generates notification noise. You can always manually rerun Github actions through the web interface.
@vladimyr this is all great stuff that you're pointing out, but as it doesn't actually improve the app from the users's perspective (i.e. performance, etc.) what do you think about ignoring all of these tweaks for this PR and saving them for another PR? I plan to open a new PR within the next couple days full of improvements I've noticed could be made and I think these would—as they're not strictly related to the os → platform change—be better for that PR.
I have a lot more things in mind than you're pointing out here, and IMO it makes the diff harder while not giving the code the wash it deserves. What do you think?
Yeah you're right @agnivade, funny that I didn't think about that. What about also printing a deprecation message when -o/--os are used, stating that they will be removed in a future release or after a specific date e.g. Dec 31 2021?
Should I still handle suggestions or would you prefer to take over? Because I think I wouldn't be very helpful here.
Oh, I had a message typed but forgot to hit send 🙃 I'll take over, provided you don't mind.
@vladimyr this is all great stuff that you're pointing out, but as it doesn't actually improve the app from the users's perspective (i.e. performance, etc.) what do you think about ignoring all of these tweaks for this PR and saving them for another PR? I plan to open a new PR within the next couple days full of improvements I've noticed could be made and I think these would—as they're not strictly related to the
os→platformchange—be better for that PR.I have a lot more things in mind than you're pointing out here, and IMO it makes the diff harder while not giving the code the wash it deserves. What do you think?
So here is what I think. You are right changes I proposed are transparent to the end-user and don't affect them in any possible way but they improve the state of a particular piece of codebase limited by boundaries of this patch/PR. I tend to follow Martin Fowler's school of thought there which can be summarised as - each time you touch some part of the codebase go and make it a bit better right away instead of scheduling grand refactors.
As per your second point, I fully agree that there are countless things that need to get addressed in tldr's codebase but please make that an incremental process rather than one big rewrite.
This last point of yours about making the diff harder I can just say I explicitly limited myself to not suggest too many changes and to keep those on topic. I think I did a good job there but that's obviously subjective 🙃
Also as @agnivade pointed out this is a breaking change so increasing of major version is a semver requirement.
For displaying deprecation message, we could use https://www.npmjs.com/package/depd#deprecating-property-access
For displaying deprecation message, we could use https://www.npmjs.com/package/depd#deprecating-property-access
Looks nice, I'll try it. The message might be developer-oriented rather user-oriented, but we'll see.
So here is what I think. ... ... This last point of yours about making the diff harder I can just say I explicitly limited myself to not suggest too many changes and to keep those on topic. I think I did a good job there but that's obviously subjective 🙃
Good points, I agree with all of them. I'll apply the suggestions and keep future cleanup PRs small and limited to a particular type of fix.
I'm sure there'll be plenty of other PRs to the Node.js client here to improve things in too lol
(did we update the Node.js client to automatically convert the page name to lowercase?)
No, I don't think so! Another spec compliance issue! 😄 I will gladly take care of that ASAP once this PR is merged.
Oh, sorry, you're right. I just saw that the PR hasn't had activity for some time.
I think this PR is indeed ready for review, @agnivade
It still lacks deprecation notice and as @agnivade already said simply replaces --os by --platform thus introducing a breaking change. :warning: ie this needs more work/love...
