Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add silent flag to the cli command to supress game logs (#46) #82

Merged
merged 8 commits into from
May 21, 2018

Conversation

xaviserrag
Copy link
Contributor

@xaviserrag xaviserrag commented May 12, 2018

Modified the CLI project to accept a --silent value so the gameplay skips the play logs.

Had to use -m (mute) for the short version of the command since -s was already used, open to suggestions on using other options.

Saw this project yesterday and I loved it, keep up the good work! ;)

Closes #46

@codecov-io
Copy link

codecov-io commented May 12, 2018

Codecov Report

Merging #82 into master will decrease coverage by 0.07%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   91.13%   91.05%   -0.08%     
==========================================
  Files          81       81              
  Lines        1049     1051       +2     
  Branches      168      169       +1     
==========================================
+ Hits          956      957       +1     
  Misses         77       77              
- Partials       16       17       +1
Impacted Files Coverage Δ
packages/warriorjs-cli/src/parseArgs.js 100% <ø> (ø) ⬆️
packages/warriorjs-cli/src/cli.js 100% <100%> (ø) ⬆️
packages/warriorjs-cli/src/Game.js 53.16% <60%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d412e78...9450604. Read the comment docs.

Copy link
Owner

@olistic olistic left a comment

Choose a reason for hiding this comment

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

@xaviserrag Thank you for your interest in the project and for this PR! This is gonna be useful.

Some comments: go ahead and use -s for the short version of --silent as I'm working on a PR to change the --skip option (I have the feeling --silent is gonna be more used than --skip). Once I merge that PR you'll have to update this one but you can start working in the changes if you like.

@@ -31,13 +31,21 @@ class Game {
* @param {string} runDirectoryPath The directory under which to run the game.
* @param {number} practiceLevel The level to practice.
* @param {boolean} skipInput Whether to skip user input or not.
* @param {boolean} silentGameplay Whether to skip displaying game logs or not.
Copy link
Owner

Choose a reason for hiding this comment

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

@xaviserrag What do you think of silencePlay instead of silentGameplay?

@@ -7,8 +7,8 @@ import parseArgs from './parseArgs';
* @param {string[]} args The command line arguments.
*/
async function run(args) {
const { directory, level, skip, time } = parseArgs(args);
const game = new Game(directory, level, skip, time);
const { directory, level, skip, time, silent } = parseArgs(args);
Copy link
Owner

Choose a reason for hiding this comment

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

Let's keep the alphabetical order here 👆

@@ -35,6 +35,11 @@ function parseArgs(args) {
describe: 'Skip user input',
type: 'boolean',
},
m: {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's use s here, as per my general comment.

@@ -35,6 +35,11 @@ function parseArgs(args) {
describe: 'Skip user input',
type: 'boolean',
},
m: {
alias: 'silent',
describe: 'Skip displaying game logs',
Copy link
Owner

Choose a reason for hiding this comment

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

What we are suppressing is the play log, what do you think we change this to "Suppress play log"?

@olistic
Copy link
Owner

olistic commented May 13, 2018

@xaviserrag Also, if you can add tests to this to at least preserve the coverage, that'd be awesome!

@olistic
Copy link
Owner

olistic commented May 13, 2018

@xaviserrag Just writing to let you know that --skip (and -s) where removed in favor of --yes (#93). -s is now available for you to use with --silent.

@xaviserrag
Copy link
Contributor Author

Sounds good! Will go through the changes soon!

@olistic
Copy link
Owner

olistic commented May 18, 2018

@xaviserrag I just noticed that the documentation needs to be updated as well (it lists the options in alphabetical order).

@olistic olistic force-pushed the add-silent-flag branch 2 times, most recently from 38c3815 to 1ab31c6 Compare May 21, 2018 03:05
Copy link
Owner

@olistic olistic left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

After merging #144, I took the liberty to commit to your branch and undo the changes to the package.json files. Then I added a tiny commit with some cosmetic changes.

@olistic olistic merged commit 37c082b into olistic:master May 21, 2018
@xaviserrag
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants