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

Update commander to the latest version #138

Merged
merged 1 commit into from
Mar 8, 2021
Merged

Conversation

GeoSot
Copy link
Collaborator

@GeoSot GeoSot commented Mar 8, 2021

Refs #100

Closes #128 by superseding it

@GeoSot GeoSot force-pushed the update-commander-version branch from fd1b5f1 to b8c67a9 Compare March 8, 2021 11:49
@XhmikosR XhmikosR force-pushed the update-commander-version branch from 1bfd2f9 to 3488273 Compare March 8, 2021 12:38
@XhmikosR XhmikosR force-pushed the update-commander-version branch from 3488273 to f3f9d87 Compare March 8, 2021 12:44
cli.js Outdated
const chalk = require('chalk');
const { version } = require('./package.json');
const fusv = require('.');

commander
program
.usage('[options] <folders...>')
.version(version, '-v, --version')
.option('-i, --ignore <ignoredVars>', 'ignore variables, comma separated')
Copy link
Owner

Choose a reason for hiding this comment

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

@GeoSot this still doesn't use the variadic option ... https://github.com/tj/commander.js/#variadic-option
Wouldn't using this simplify things?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to keep things the same as possible.
It can be done. Although the $ prefix in each var, will have to be escaped.
I propose to leave it as it is and we can change it in a next pr

Copy link
Owner

Choose a reason for hiding this comment

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

Not our job to escape it, I think? I tested it with the default Windows cmd and it seems to work fine here.

C:\Users\xmr\Desktop\find-unused-sass-variables>npm run check -- --ignore $unused

> find-unused-sass-variables@3.1.0 check C:\Users\xmr\Desktop\find-unused-sass-variables
> node ./cli.js tests/ non-existent-folder/ -i "$a" "--ignore" "$unused"

Looking for unused variables
Finding unused variables in "C:\Users\xmr\Desktop\find-unused-sass-variables\tests"...
9 total variables.
6 are not used!
Variable $b is not being used!
Variable $black is not being used!
Variable $ignored-variable is not being used!
Variable $enabled-variable is not being used!
Variable $nestedVar is not being used!
Variable $nestNestedVar is not being used!
Finding unused variables in "C:\Users\xmr\Desktop\find-unused-sass-variables\non-existent-folder"...
"C:\Users\xmr\Desktop\find-unused-sass-variables\non-existent-folder": Not a valid directory!

Copy link
Collaborator Author

@GeoSot GeoSot Mar 8, 2021

Choose a reason for hiding this comment

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

not in linux :/

> node ./cli.js tests/ non-existent-folder/ -i "$a" "--ignore" "$unused"

{ ignore: [ '', '' ] }
> node ./cli.js tests/ non-existent-folder/ -i '$a' --ignore '$unused' 'b'

{ ignore: [ '$a', '$unused', 'b' ] }

Copy link
Owner

Choose a reason for hiding this comment

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

All right, I'll revert the variadic change and we can tackle it along with the '' in another PR :)

@XhmikosR XhmikosR force-pushed the update-commander-version branch from f3f9d87 to a64c198 Compare March 8, 2021 12:49
Also, refactor cli code a bit
@XhmikosR XhmikosR force-pushed the update-commander-version branch from 317fe6c to 5d1b8be Compare March 8, 2021 13:15
@XhmikosR XhmikosR added the dependencies Pull requests that update a dependency file label Mar 8, 2021
@XhmikosR XhmikosR merged commit e3e6b7b into master Mar 8, 2021
@XhmikosR XhmikosR deleted the update-commander-version branch March 8, 2021 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants