Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

Add ability to sort advices based on cli arguments #223

Merged

Conversation

lapa182
Copy link

@lapa182 lapa182 commented Jan 13, 2017

  • Check that your change/fix has corresponding unit tests
  • Verify that the test works by running npm test and test linting by npm run lint
  • Squash commits so it looks sane

Description

Add the ability to sort advices list based on the cli argument (--sortBy=), if no argument is passed set name as default. This is the pull request for the feature #222

@lapa182 lapa182 changed the title Added ability to sort advices based on cli arguments Add ability to sort advices based on cli arguments Jan 13, 2017
@lapa182 lapa182 force-pushed the feature/add-sortoption-adviceInfo#222 branch 2 times, most recently from 12be68c to dbd5eae Compare January 13, 2017 15:43
Copy link
Member

@tobli tobli left a comment

Choose a reason for hiding this comment

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

👍
Great stuff, just some tweaks.

lib/table.js Outdated
@@ -3,6 +3,8 @@
var wrap = require('word-wrap');

var Table = require('cli-table2'),
argv = require('yargs').argv,
Copy link
Member

Choose a reason for hiding this comment

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

Put the definition of the sortBy option into cli.js (with documentation). I suggest a choice of 'name' and 'score', with a default of 'name'. Then make sortProperty an argument to getNewTable.

lib/table.js Outdated
@@ -3,6 +3,8 @@
var wrap = require('word-wrap');

var Table = require('cli-table2'),
argv = require('yargs').argv,
_ = require('lodash'),
Copy link
Member

Choose a reason for hiding this comment

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

'lodash' is not a dependency in package.json. Just depend on 'lodash.sortBy' (making sortProperty an argument you can skip _.isUndefined).

lib/table.js Outdated

let sortedAdvice;

sortedAdvice = sortProperties(adviceList, argv.sortBy);
Copy link
Member

Choose a reason for hiding this comment

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

while the changes above, you can simplify this to

sortedAdvice = sortBy(adviceList, sortProperty);

Added ability to sort advices based on cli arguments
@lapa182 lapa182 force-pushed the feature/add-sortoption-adviceInfo#222 branch from 20e683b to cc0eb53 Compare February 21, 2017 12:26
@lapa182
Copy link
Author

lapa182 commented Feb 21, 2017

@tobli done :D!

@tobli tobli merged commit 6a76107 into sitespeedio:master Feb 21, 2017
@tobli
Copy link
Member

tobli commented Feb 21, 2017

Thanks!

@lapa182 lapa182 deleted the feature/add-sortoption-adviceInfo#222 branch March 13, 2017 15:41
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.

2 participants