Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Refactor bin/lisky to increase readability - Closes #433 #439

Closed
wants to merge 7 commits into from

Conversation

shuse2
Copy link
Contributor

@shuse2 shuse2 commented Feb 19, 2018

Closes #433

Merge After #438

@shuse2 shuse2 self-assigned this Feb 19, 2018
@shuse2 shuse2 force-pushed the 433-refactor_bin_lisky branch 2 times, most recently from 0f72cbf to 6bfea78 Compare February 19, 2018 15:49
@willclarktech willclarktech changed the base branch from 1.0.0 to 1.1.0 March 16, 2018 09:35
Copy link
Contributor

@willclarktech willclarktech left a comment

Choose a reason for hiding this comment

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

We should add bin/lisky to the npm lint script, because ESLint won't find it automatically.

@@ -1,8 +1,11 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This file can be removed now.

bin/lisky Outdated
const semver = require('semver');
const packageJSON = require('../package.json');

// eslint-disable-next-line import/extensions,import/no-unresolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a space after ,

bin/lisky Outdated
const semver = require('semver');
const packageJSON = require('../package.json');

// eslint-disable-next-line import/extensions,import/no-unresolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a good idea? What if e.g. dist/execFile.js gets a new filename?

bin/lisky Outdated
return null;
};

module.export = run();
Copy link
Contributor

Choose a reason for hiding this comment

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

module.exports is more standard.

bin/lisky Outdated
const packageJSON = require('../package.json');

// eslint-disable-next-line import/extensions,import/no-unresolved
const lisky = require('../dist').default;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't be required here. Try ./bin/lisky --version or ./bin/lisky clean to see what I mean.

bin/lisky Outdated
}
};

const handleBasicCommands = (command, lockFilePath, version) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest handleShallowCommands?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basic might not be the best word, but why shallow?

bin/lisky Outdated
}

const commandArgIsFilePath = isFileInput(nonInteractiveCommandArg);
let fileHandled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer a solution without let.

bin/lisky Outdated
exit();
}

const commandArgIsFilePath = isFileInput(nonInteractiveCommandArg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest phrasing this in terms of something like isKnownCommand?

bin/lisky Outdated
try {
execFile(liskyInstnce, command, options, exitFn);
return true;
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer verbose error

bin/lisky Outdated
const commandArgIsFilePath = isFileInput(nonInteractiveCommandArg);
let fileHandled = false;
if (commandArgIsFilePath) {
const nonInteractiveOptions = process.argv.slice(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be defined with the other process.argv things.

@shuse2 shuse2 force-pushed the 433-refactor_bin_lisky branch from 6bfea78 to bdcc955 Compare March 19, 2018 15:16
@shuse2
Copy link
Contributor Author

shuse2 commented May 30, 2018

Closing because #524 will delete the changes we make here

@shuse2 shuse2 closed this May 30, 2018
@shuse2 shuse2 deleted the 433-refactor_bin_lisky branch May 30, 2018 15:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants