Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
wants to merge 5 commits into from
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
@@ -21,7 +21,7 @@ npm install -g tldr
To see tldr pages:

- `tldr <command>` show examples for this command
- `tldr <command> --os=<platform>` show command page for the given platform (`linux`, `osx`, `sunos`, `windows`)
- `tldr <command> --platform=<platform>` show command page for the given platform (`linux`, `osx`, `sunos`, `windows`)
- `tldr --search "<query>"` search all pages for the query
- `tldr --linux <command>` show command page for Linux
- `tldr --osx <command>` show command page for OSX
@@ -91,7 +91,7 @@ you can put it in the config file:
The default platform value can be overwritten with command-line option:

```shell
tldr du --os=osx
tldr du --platform=osx
```

As a contributor, you can also point to your own fork containing the `tldr.zip` file. The file is just a zipped version of the entire tldr repo:
8 changes: 4 additions & 4 deletions bin/completion/bash/tldr
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@

BUILTIN_THEMES="single base16 ocean"

OS_TYPES="linux osx sunos windows"
PLATFORMS="linux osx sunos windows"

OPTIONS='-V
--version
@@ -20,7 +20,7 @@ OPTIONS='-V
--render
-m
--markdown
-o
-p
--linux
--osx
--sunos
@@ -56,8 +56,8 @@ function _tldr_autocomplete {
COMPREPLY=(`compgen -f $cur`)
;;

-o|--os)
COMPREPLY=(`compgen -W "$OS_TYPES" $cur`)
-p|--platform)
COMPREPLY=(`compgen -W "$PLATFORMS" $cur`)
;;

-t|--theme)
14 changes: 7 additions & 7 deletions bin/completion/zsh/_tldr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
#compdef tldr

local -a pages oses
local -a pages platforms
pages=$(tldr -a1)
oses='( linux osx sunos windows )'
platforms='( linux osx sunos windows )'

_arguments \
'(- *)'{-h,--help}'[show help]' \
@@ -15,11 +15,11 @@ _arguments \
'(- *)'{-e,--random-example}'[show a random example]' \
'(- *)'{-m,--markdown}'[show the original markdown format page]' \
'(-f --render)'{-f,--render}'[render a specific markdown file]:markdown file:_files -/' \
'(-o --os)'{-o,--os}"[override operating system]:os:${oses}" \
'--linux[override operating system with Linux]' \
'--osx[override operating system with OSX]' \
'--sunos[override operating system with SunOS]' \
'--windows[override operating system with Windows]' \
'(-p --platform)'{-p,--platform}"[override platform]:platform:${platforms}" \
'--linux[override platform with Linux]' \
'--osx[override platform with OSX]' \
'--sunos[override platform with SunOS]' \
'--windows[override platform with Windows]' \
'(- *)'{-u,--update}'[update local cache]' \
'(- *)'{-c,--clear-cache}'[clear local cache]' \
"*:page:(${pages})" && return 0
28 changes: 14 additions & 14 deletions bin/tldr
Original file line number Diff line number Diff line change
@@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@vladimyr vladimyr Mar 28, 2021

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@bl-ue bl-ue Mar 28, 2021

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 😂


program
.version(pkg.version, '-v, --version')
@@ -20,11 +20,11 @@ program
.option('-e, --random-example', 'Show a random example')
.option('-f, --render [file]', 'Render a specific markdown [file]')
.option('-m, --markdown', 'Output in markdown format')
.option('-o, --os [type]', 'Override the operating system [linux, osx, sunos, windows]')
.option('--linux', 'Override the operating system with Linux')
.option('--osx', 'Override the operating system with OSX')
.option('--sunos', 'Override the operating system with SunOS')
.option('--windows', 'Override the operating system with Windows')
.option('-p, --platform [platform]', 'Override platform [linux, osx, sunos, windows]')
.option('--linux', 'Override platform with Linux')
.option('--osx', 'Override platform with OSX')
.option('--sunos', 'Override platform with SunOS')
.option('--windows', 'Override platform with Windows')
.option('-t, --theme [theme]', 'Color theme (simple, base16, ocean)')
.option('-s, --search [keywords]', 'Search pages using keywords')
//
@@ -37,7 +37,7 @@ const help = `
Examples:

$ tldr tar
$ tldr du --os=linux
$ tldr du --platform=linux
$ tldr --search "create symbolic link to file"
$ tldr --list
$ tldr --list-all
@@ -61,25 +61,25 @@ program.on('--help', () => {
program.parse(process.argv);

if (program.linux) {
program.os = 'linux';
program.platform = 'linux';
}

if (program.osx) {
program.os = 'osx';
program.platform = 'osx';
}

if (program.sunos) {
program.os = 'sunos';
program.platform = 'sunos';
}

if (program.windows) {
program.os = 'windows';
program.platform = 'windows';
}

let cfg = config.get();
if (program.os) {
if (platform.isSupported(program.os)) {
cfg.platform = program.os;
if (program.platform) {
if (platformUtils.isSupported(program.platform)) {
cfg.platform = program.platform;
}
}

4 changes: 2 additions & 2 deletions lib/cache.js
Original file line number Diff line number Diff line change
@@ -4,7 +4,7 @@ const fs = require('fs-extra');
const os = require('os');
const path = require('path');
const remote = require('./remote');
const platform = require('./platform');
const platformUtils = require('./platformUtils');
const index = require('./index');
const utils = require('./utils');

@@ -21,7 +21,7 @@ class Cache {
}

getPage(page) {
let preferredPlatform = platform.getPreferredPlatformFolder(this.config);
let preferredPlatform = platformUtils.getPreferredPlatformFolder(this.config);
const preferredLanguage = process.env.LANG || 'en';
return index.findPage(page, preferredPlatform, preferredLanguage)
.then((folder) => {
10 changes: 5 additions & 5 deletions lib/config.js
Original file line number Diff line number Diff line change
@@ -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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


exports.get = () => {
const DEFAULT = path.join(__dirname, '..', 'config.json');
@@ -37,10 +38,9 @@ exports.get = () => {
return merged;
};

function validatePlatform(os) {
let platform = require('./platform');
if (os && !platform.isSupported(os)) {
return 'Unsupported platform : ' + os;
function validatePlatform(platform) {
if (platform && !platformUtils.isSupported(platform)) {
return 'Unsupported platform : ' + platform;
}
return null;
}
@@ -100,4 +100,4 @@ function validateThemeItem(field, key) {
return null;
}
return errMsg.join('\n');
}
}
20 changes: 10 additions & 10 deletions lib/index.js
Original file line number Diff line number Diff line change
@@ -59,12 +59,12 @@ function findPage(page, preferredPlatform, preferredLanguage) {
targetPlatform = 'common';
} else if (targets.length > 0 && hasLang(targets, preferredLanguage)) {
targetLanguage = preferredLanguage;
targetPlatform = targets[0].os;
console.log(`Command ${page} does not exist for the host platform. Displaying the page from ${targets[0].os} platform`);
targetPlatform = targets[0].platform;
console.log(`Command ${page} does not exist for the host platform. Displaying the page from ${targetPlatform} platform`);
} else if (targets.length > 0 && hasLang(targets, 'en')) {
targetLanguage = 'en';
targetPlatform = targets[0].os;
console.log(`Command ${page} does not exist for the host platform. Displaying the page from ${targets[0].os} platform`);
targetPlatform = targets[0].platform;
console.log(`Command ${page} does not exist for the host platform. Displaying the page from ${targetPlatorm} platform`);
}

if (!targetLanguage && !targetPlatform) {
@@ -81,7 +81,7 @@ function findPage(page, preferredPlatform, preferredLanguage) {

function hasPlatformLang(targets, preferredPlatform, preferredLanguage) {
return targets.some((t) => {
return t.os === preferredPlatform && t.language === preferredLanguage;
return t.platform === preferredPlatform && t.language === preferredLanguage;
});
}

@@ -116,7 +116,7 @@ function commandsFor(platform) {
let commands = Object.keys(idx)
.filter((cmd) => {
let targets = idx[cmd].targets;
let platforms = targets.map((t) => {return t.os;});
let platforms = targets.map((t) => {return t.platform;});
return platforms.indexOf(platform) !== -1 || platforms.indexOf('common') !== -1;
})
.sort();
@@ -187,24 +187,24 @@ function buildShortPagesIndex() {
.then((files) => {
files = files.filter(utils.isPage);
let reducer = (index, file) => {
let os = utils.parsePlatform(file);
let platform = utils.parsePlatform(file);
let page = utils.parsePagename(file);
let language = utils.parseLanguage(file);
if (index[page]) {
let targets = index[page].targets;
let needsPush = true;
for (const target of targets) {
if (target.platform === os && target.language === language) {
if (target.platform === platform && target.language === language) {
needsPush = false;
continue;
}
}
if (needsPush) {
targets.push({'os': os, 'language': language});
targets.push({ platform, language });
index[page].targets = targets;
}
} else {
index[page] = {targets: [{'os': os, 'language': language}]};
index[page] = { targets: [{ platform, language }] };
}

return index;
File renamed without changes.
8 changes: 4 additions & 4 deletions lib/search.js
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@ const natural = require('natural');
const config = require('./config');
const utils = require('./utils');
const index = require('./index');
const platform = require('./platform');
const platformUtils = require('./platformUtils');

const CACHE_FOLDER = path.join(config.get().cache, 'cache');

@@ -189,16 +189,16 @@ exports.printResults = (results, config) => {
// Prints the passed results to the console.
// If the command is not available for the current platform,
// it lists the available platforms instead.
// Example: tldr --search print directory tree --os sunos prints:
// Example: tldr --search print directory tree --platform sunos prints:
// $ tree (Available on: linux, osx)
index.getShortIndex().then((shortIndex) => {
let outputs = new Set();
let preferredPlatform = platform.getPreferredPlatform(config);
let preferredPlatform = platformUtils.getPreferredPlatform(config);
results.forEach((elem) => {
let cmdname = utils.parsePagename(elem.file);
let output = ' $ ' + cmdname;
let targets = shortIndex[cmdname]['targets'];
let platforms = targets.map((t) => {return t.os;});
let platforms = targets.map((t) => {return t.platform;});
if (platforms.indexOf('common') === -1 && platforms.indexOf(preferredPlatform) === -1) {
output += ' (Available on: ' + platforms.join(', ') + ')';
}
14 changes: 7 additions & 7 deletions lib/tldr.js
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@ const ms = require('ms');
const ora = require('ora');
const Cache = require('./cache');
const search = require('./search');
const platform = require('./platform');
const platformUtils = require('./platformUtils');
const messages = require('./messages');
const parser = require('./parser');
const render = require('./render');
@@ -22,8 +22,8 @@ class Tldr {
}

list(singleColumn) {
let os = platform.getPreferredPlatformFolder(this.config);
return index.commandsFor(os)
let platform = platformUtils.getPreferredPlatformFolder(this.config);
return index.commandsFor(platform)
.then((commands) => {
return this.printPages(commands, singleColumn);
});
@@ -41,8 +41,8 @@ class Tldr {
}

random(options) {
let os = platform.getPreferredPlatformFolder(this.config);
return index.commandsFor(os)
let platform = platformUtils.getPreferredPlatformFolder(this.config);
return index.commandsFor(platform)
.then((pages) => {
if (pages.length === 0) {
throw new Error(messages.emptyCache());
@@ -57,8 +57,8 @@ class Tldr {
}

randomExample() {
let os = platform.getPreferredPlatformFolder(this.config);
return index.commandsFor(os)
let platform = platformUtils.getPreferredPlatformFolder(this.config);
return index.commandsFor(platform)
.then((pages) => {
if (pages.length === 0) {
throw new Error(messages.emptyCache());
14 changes: 7 additions & 7 deletions test/cache.spec.js
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@ const path = require('path');
const fs = require('fs-extra');
const index = require('../lib/index');
const remote = require('../lib/remote');
const platform = require('../lib/platform');
const platformUtils = require('../lib/platformUtils');


describe('Cache', () => {
@@ -79,44 +79,44 @@ describe('Cache', () => {

it('should return page contents for ls', () => {
sinon.stub(fs, 'readFile').resolves('# ls\n> ls page');
sinon.stub(platform, 'getPreferredPlatformFolder').returns('osx');
sinon.stub(platformUtils, 'getPreferredPlatformFolder').returns('osx');
sinon.stub(index, 'findPage').resolves('osx');
const cache = new Cache(config.get());
return cache.getPage('ls')
.then((content) => {
should.exist(content);
content.should.startWith('# ls');
fs.readFile.restore();
platform.getPreferredPlatformFolder.restore();
platformUtils.getPreferredPlatformFolder.restore();
index.findPage.restore();
});
});

it('should return empty contents for svcs on OSX', () =>{
sinon.stub(fs, 'readFile').resolves('# svcs\n> svcs');
sinon.stub(platform, 'getPreferredPlatformFolder').returns('osx');
sinon.stub(platformUtils, 'getPreferredPlatformFolder').returns('osx');
sinon.stub(index, 'findPage').resolves(null);
const cache = new Cache(config.get());
return cache.getPage('svc')
.then((content) => {
should.not.exist(content);
fs.readFile.restore();
platform.getPreferredPlatformFolder.restore();
platformUtils.getPreferredPlatformFolder.restore();
index.findPage.restore();
});
});

it('should return page contents for svcs on SunOS', () => {
sinon.stub(fs, 'readFile').resolves('# svcs\n> svcs');
sinon.stub(platform, 'getPreferredPlatformFolder').returns('sunos');
sinon.stub(platformUtils, 'getPreferredPlatformFolder').returns('sunos');
sinon.stub(index, 'findPage').resolves('svcs');
const cache = new Cache(config.get());
return cache.getPage('svcs')
.then((content) => {
should.exist(content);
content.should.startWith('# svcs');
fs.readFile.restore();
platform.getPreferredPlatformFolder.restore();
platformUtils.getPreferredPlatformFolder.restore();
index.findPage.restore();
});
});
Loading