change `-o/--os` to `-p/--platform` #337

marchersimon · 2021-03-26T20:05:04Z

Closes #336

marchersimon · 2021-03-26T20:18:34Z

Any idea what's wrong with the failed test here?

bl-ue · 2021-03-26T20:27:19Z

@marchersimon don't edit this PR yet—I'm making some changes.

bl-ue · 2021-03-26T20:34:01Z

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.

bl-ue · 2021-03-26T20:55:59Z

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 👍🏻

marchersimon · 2021-03-26T21:04:08Z

Oh, sorry, I just blindly replaced -o and --os without checking if they occur somewhere else. Thanks for the fix

bl-ue · 2021-03-26T21:13:11Z

There we go, the checks are all green.

bl-ue

Funny that the most official of all the official clients doesn't even comply with the spec (in a big way!) 😄

vladimyr

Looks good overall, just a few suggested changes 👍
@marchersimon Thanks for addressing it.

vladimyr · 2021-03-27T00:14:45Z

bin/tldr

@@ -4,7 +4,7 @@ const program = require('commander');
 const pkg = require('../package');
 const Tldr = require('../lib/tldr');
 const config = require('../lib/config');
-const platform = require('../lib/platform');
+const platformUtils = require('../lib/platformUtils');


Suggested change

const platformUtils = require('../lib/platformUtils');

const Platform = require('../lib/platform');

Makes diff smaller and addresses name clash in a fairly standard manner.

You think so? I mean to ask you in the extended commit message of 5e1b052

Well, I've seen it more than once in various node codebases but that is anecdotal evidence. Maybe going with

const platforms = require('../lib/platforms');

strikes the right balance?

I don't have strong opinions regarding either, but do you think platformUtils it not standard?

utils and helpers are well-known wildcards conveying absolutely zero meaning so some folks consider naming things using them as an antipattern. In this case, it has been used as a name-clash avoiding suffix so it isn't a problem except it hurts my eyes 🙈
But let's take a step back, what exactly mandates rename in the first place, I failed to spot that name clash we are trying to avoid here? 😕

That's the same situation as requiring the path module and then having a variable named path. In most cases platform is really accessed as a property of the program object which makes it a non-issue. And if there are some shadowing instances they should be addressed by renaming the local variable instead, for example, targetPlatform or selectedPlatform.

Yeah, you're thinking the right way. I guess I'm busy so I didn't think a lot about this. I'll change the name back to platform.js 👍🏻

What would you do about cases like this?

function validatePlatform(platform) { if (platform && !platformUtils.isSupported(platform)) { return 'Unsupported platform : ' + platform; } return null; }

Personally I'd pick p as a parameter name, but that doesn't strike my memory as conventional JS (I'm not working on much js lately)

Well if you do validation then one thing is for sure, you are doing it on input 😉

Yeah, you're thinking the right way. I guess I'm busy so I didn't think a lot about this. I'll change the name back to platform.js 👍🏻

Watching me doing things you would soon realize that most of the time it seems like I'm not thinking at all 😂

bin/tldr

vladimyr · 2021-03-27T00:22:13Z

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.

bl-ue · 2021-03-27T00:25:26Z

You're right; I realized that after I closed/reopened it 😂

vladimyr · 2021-03-27T00:51:23Z

lib/config.js

@@ -4,6 +4,7 @@ const defaults = require('lodash/defaults');
 const fs = require('fs');
 const path = require('path');
 const osHomedir = require('os').homedir;
+let platformUtils = require('./platformUtils');


Suggested change

let platformUtils = require('./platformUtils');

const platformUtils = require('./platformUtils');

lib/index.js

bl-ue · 2021-03-27T01:56:17Z

@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?

agnivade

Thanks for the PR. However, changing an option outright is a huge breaking change. Let's add the --platform option which can be a synonym for --os so that existing users aren't affected.

bl-ue · 2021-03-27T12:04:51Z

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?

agnivade · 2021-03-27T14:36:32Z

Sounds reasonable. It can be removed in the next major release.

marchersimon · 2021-03-27T19:56:36Z

Should I still handle suggestions or would you prefer to take over? Because I think I wouldn't be very helpful here.

bl-ue · 2021-03-27T19:59:50Z

Oh, I had a message typed but forgot to hit send 🙃 I'll take over, provided you don't mind.

marchersimon · 2021-03-27T20:01:47Z

Sure, thank you

vladimyr · 2021-03-28T22:03:45Z

@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?

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 🙃

vladimyr · 2021-03-28T22:07:11Z

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

bl-ue · 2021-03-28T22:10:56Z

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.

Co-authored-by: Dario Vladović <d.vladimyr@gmail.com>

lib/index.js

sbrl · 2021-03-30T22:46:47Z

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?)

bl-ue · 2021-03-30T23:49:00Z

No, I don't think so! Another spec compliance issue! 😄 I will gladly take care of that ASAP once this PR is merged.

agnivade · 2021-04-16T03:37:12Z

Is this ready for review? This still seems to replace --os by --platform.

marchersimon · 2021-04-16T09:00:56Z

Oh, sorry, you're right. I just saw that the PR hasn't had activity for some time.

sbrl · 2021-04-22T20:51:25Z

I think this PR is indeed ready for review, @agnivade

vladimyr · 2021-04-23T05:06:15Z

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. ⚠️ ie this needs more work/love...

MasterOdin · 2023-10-13T04:55:10Z

Closing this in favor of #421

change -o to -p and --os to --platform

abbb56a

bl-ue added the enhancement label Mar 26, 2021

bl-ue changed the title ~~change -o to -p and --os to --platform~~ change -o/--os to -p/--platform Mar 26, 2021

bl-ue closed this Mar 26, 2021

bl-ue reopened this Mar 26, 2021

bl-ue added 2 commits March 26, 2021 16:41

rename platform.js to platformUtils.js

5e1b052

os/operating system -> platform

39ce948

bl-ue requested review from vladimyr and agnivade March 26, 2021 20:56

bl-ue approved these changes Mar 26, 2021

View reviewed changes

vladimyr reviewed Mar 27, 2021

View reviewed changes

vladimyr requested changes Mar 27, 2021

View reviewed changes

agnivade requested changes Mar 27, 2021

View reviewed changes

Apply suggestions from code review

881ddd8

Co-authored-by: Dario Vladović <d.vladimyr@gmail.com>

vladimyr reviewed Mar 28, 2021

View reviewed changes

lib/index.js Outdated Show resolved Hide resolved

Update lib/index.js

b532a0f

bl-ue mentioned this pull request Apr 14, 2021

tldr: edit examples tldr-pages/tldr#5508

Merged

marchersimon requested review from agnivade and vladimyr April 15, 2021 18:45

sbrl mentioned this pull request Oct 8, 2021

tldr: fix typo tldr-pages/tldr#6869

Closed

agnivade removed their request for review November 15, 2021 04:51

reinhart1010 mentioned this pull request Aug 4, 2022

windows/curl,windows/wget: fix tldr client param inconsistencies tldr-pages/tldr#8297

Merged

5 tasks

pixelcmtd mentioned this pull request Oct 24, 2022

rename --os/-o to --platform/-p #390

Closed

4 tasks

kbdharun changed the base branch from master to main October 3, 2023 17:12

kbdharun mentioned this pull request Oct 5, 2023

feat (3.4.0): add platform option, Android and BSD platforms #421

Merged

4 tasks

MasterOdin closed this Oct 13, 2023

	const platformUtils = require('../lib/platformUtils');
	const Platform = require('../lib/platform');

	let platformUtils = require('./platformUtils');
	const platformUtils = require('./platformUtils');

change -o/--os to -p/--platform #337

change -o/--os to -p/--platform #337

Conversation

marchersimon commented Mar 26, 2021

marchersimon commented Mar 26, 2021

bl-ue commented Mar 26, 2021

bl-ue commented Mar 26, 2021 • edited Loading

bl-ue commented Mar 26, 2021

marchersimon commented Mar 26, 2021

bl-ue commented Mar 26, 2021

bl-ue left a comment

Choose a reason for hiding this comment

vladimyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimyr Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bl-ue Mar 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimyr commented Mar 27, 2021 • edited Loading

bl-ue commented Mar 27, 2021

Choose a reason for hiding this comment

bl-ue commented Mar 27, 2021 • edited Loading

agnivade left a comment

Choose a reason for hiding this comment

bl-ue commented Mar 27, 2021

agnivade commented Mar 27, 2021

marchersimon commented Mar 27, 2021

bl-ue commented Mar 27, 2021 • edited Loading

marchersimon commented Mar 27, 2021

vladimyr commented Mar 28, 2021

vladimyr commented Mar 28, 2021

bl-ue commented Mar 28, 2021

sbrl commented Mar 30, 2021

bl-ue commented Mar 30, 2021 • edited Loading

agnivade commented Apr 16, 2021

marchersimon commented Apr 16, 2021

sbrl commented Apr 22, 2021

vladimyr commented Apr 23, 2021

MasterOdin commented Oct 13, 2023

change `-o/--os` to `-p/--platform` #337

change `-o/--os` to `-p/--platform` #337

bl-ue commented Mar 26, 2021 •

edited

Loading

vladimyr Mar 28, 2021 •

edited

Loading

bl-ue Mar 28, 2021 •

edited

Loading

vladimyr commented Mar 27, 2021 •

edited

Loading

bl-ue commented Mar 27, 2021 •

edited

Loading

bl-ue commented Mar 27, 2021 •

edited

Loading

bl-ue commented Mar 30, 2021 •

edited

Loading