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

Allow to ignore some vars #24

Merged
merged 2 commits into from
Oct 29, 2018
Merged

Allow to ignore some vars #24

merged 2 commits into from
Oct 29, 2018

Conversation

MartijnCuppens
Copy link
Contributor

@MartijnCuppens MartijnCuppens commented Sep 23, 2018

With this PR it's possible to ignore some variables.
Usage:

fusv folder --ignore '$my-var,$my-second-var'

Closes #18

@MartijnCuppens MartijnCuppens changed the title #18: Allow to ignore some vars Allow to ignore some vars Sep 23, 2018
index.js Outdated
@@ -11,7 +11,9 @@ function regExpQuote(str) {
return str.replace(/[-\\^$*+?.()|[\]{}]/g, '\\$&');
}

function findUnusedVars(strDir) {
function findUnusedVars(strDir, opts) {
opts.ignore = opts.ignore || [];
Copy link
Owner

Choose a reason for hiding this comment

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

Are we sure opts will always exist? Maybe Object.assign would be a better choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't assume that indeed. I don't know for sure how I can use Object.assign for this, can you help me out with that?

Copy link
Collaborator

@Johann-S Johann-S Sep 23, 2018

Choose a reason for hiding this comment

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

const option = Object.assign({}, opt)

Should do the trick

EDIT:

A better fix would be creating a default option something like this:

const defaultOption = {
  ignore: '',
};

And after that you can do:

const option = Object.assign(defaultOption, opt);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't that still trigger an error on L41 if opts is not defined?

.filter((variable) => !opts.ignore.includes(variable));

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we set ignore to an empty array in defaultOption nope see:

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't notice the edit you made in the comment. I've now changed this.

@@ -1,4 +1,5 @@
$white: #fff !default;
$a: 10px;
$b : 20px;
$unused : #000;
$b: 20px;
Copy link
Owner

Choose a reason for hiding this comment

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

The space before : was intentional before we switched to postcss we were using a regex and this could fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, ok. I reverted this change

@XhmikosR
Copy link
Owner

@Johann-S: if you have any free time in the weekend, could you please have a quick look at this? Basically check what type we are passing.

cli.js Show resolved Hide resolved
Copy link
Collaborator

@Johann-S Johann-S left a comment

Choose a reason for hiding this comment

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

Our cli.js should convert ignore list into an array (that's what we do)
But our index.js shouldn't allow string on the ignore key, so there is no need to convert string here and we'll be fine 👍

@XhmikosR
Copy link
Owner

@Johann-S see if you can sort all of this because I see now why @MartijnCuppens wanted this so much :P https://travis-ci.org/twbs/bootstrap/builds/444348045

@Johann-S
Copy link
Collaborator

Added a check to ensure we work on an array

cli.js Show resolved Hide resolved
@XhmikosR XhmikosR merged commit 6da32e7 into XhmikosR:master Oct 29, 2018
@MartijnCuppens MartijnCuppens deleted the unused-variables branch October 29, 2018 11:42
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.

3 participants