Skip to content
This repository was archived by the owner on Sep 21, 2022. It is now read-only.

feat: allow to extend cli parser via CLI event#852

Merged
j0tunn merged 1 commit intomasterfrom
feat/extendable.cli
Dec 6, 2017
Merged

feat: allow to extend cli parser via CLI event#852
j0tunn merged 1 commit intomasterfrom
feat/extendable.cli

Conversation

@j0tunn
Copy link
Contributor

@j0tunn j0tunn commented Dec 5, 2017

No description provided.

let exitCode;

exports.run = () => {
const program = new Command();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

используем отдельный инстанс commander'а - это позволяет избежать проблем, когда кто-нибудь у себя в плагинах использует глобальный (который идет по умолчанию)

.action((paths, options) => runGemini('test', paths, options).done());
.action((paths, options) => mkRunFn(gemini, 'test', program)(paths, options).done());

program.command('list <key>')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

это почему-то было опцией, сделал командой

doc/events.md Outdated
@@ -1,5 +1,7 @@
# Gemini events

* `CLI` - emitted right at start, before cli is parsed. Allows to add new commands and extend help message. The event is emmited with 1 argument `parser` which is the [commander](https://github.com/tj/commander.js) instance used inside gemini itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

emmited -> emitted

lib/cli/index.js Outdated
handleUncaughtExceptions();
function mkRunFn(gemini, method, globalOpts) {
return (paths, opts) => {
opts = opts || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

lib/cli/index.js Outdated
console.log(data[key] || `Cannot list option ${key}. Available options are: sets, browsers`);
}

function collect(newValue, array) {
Copy link
Contributor

@tormozz48 tormozz48 Dec 6, 2017

Choose a reason for hiding this comment

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

function collect(newValue, array = []) {
   ...
}


function collect(newValue, array) {
array = array || [];
return array.concat(newValue);
Copy link
Contributor

Choose a reason for hiding this comment

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

Как вариант: return [newValue, ...array]

Copy link
Contributor Author

@j0tunn j0tunn Dec 6, 2017

Choose a reason for hiding this comment

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

это доклеит вперед, ну и не так чтоб код проще стал читаться

Copy link
Member

@DudaGod DudaGod left a comment

Choose a reason for hiding this comment

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

🔥

@@ -1,5 +1,7 @@
# Gemini events

* `CLI` - emitted right at start, before cli is parsed. Allows to add new commands and extend help message. The event is emitted with 1 argument `parser` which is the [commander](https://github.com/tj/commander.js) instance used inside gemini itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

я не могу сказать, что на 100 % уверен, но тире тут не должно быть:

with 1 argument – `parser`

???

.version(pkg.version)
.option('-c, --config <file>', 'config file');

const configPath = preparseOption(program, 'config');
Copy link
Contributor

Choose a reason for hiding this comment

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

а точно эту функцию нужно делать "общей"? ну типо.. чтобы она принимала вторым аргументом config?

Если она сейчас нужна только для получения пути до конфига, то может она и пусть называется parseConfigOption?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

а чем это будет лучше?

Copy link
Contributor

Choose a reason for hiding this comment

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

ну а реально это функция где-то еще, кроме как для конфига может быть использована?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

это не повод хардкодить в ней какую-то конкретную опцию. Я повторю вопрос: чем предлагаемый тобой вариант будет лучше текущего?

Copy link
Contributor

Choose a reason for hiding this comment

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

в реализации выполняется определенная магия, например, с хелпом, которая сильно специфична, кажется, только для опции config.

ответ на вопрос: изменение имени позволит не лучше понять, что делает функция не заглядывая в ее реалиазацию

console.log('');
})
.action((paths, options) => runGemini('test', paths, options).done());
.action((paths, options) => mkRunFn(gemini, 'test', program)(paths, options).done());
Copy link
Contributor

Choose a reason for hiding this comment

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

а может такое API:

.actions((paths, options) => run(gemini, 'test', paths, mergeOpts(program, options)).done())

???

Ну просто.. не тяжко ли читается функция, которая возвращает функцию, которая сразу же вызывается?

@j0tunn j0tunn force-pushed the feat/extendable.cli branch from c314df0 to 81fb51f Compare December 6, 2017 12:56
Copy link
Contributor

@rostik404 rostik404 left a comment

Choose a reason for hiding this comment

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

🔥

@j0tunn j0tunn merged commit 0dcf494 into master Dec 6, 2017
@j0tunn j0tunn deleted the feat/extendable.cli branch December 6, 2017 13:15
@j0tunn j0tunn removed the in progress label Dec 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants