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

fix: eslint no unused vars #205

Merged
merged 7 commits into from
Jan 15, 2019
Merged

fix: eslint no unused vars #205

merged 7 commits into from
Jan 15, 2019

Conversation

greg-a-smith
Copy link
Contributor

@greg-a-smith greg-a-smith commented Jan 15, 2019

Enabled the no-unused-vars eslint rule and fixed lint errors.

#169

@greg-a-smith greg-a-smith requested a review from a team January 15, 2019 04:33
@@ -55,23 +55,23 @@ describe('<MultiInput />', () => {
});

// create a default multi-input control
test('create multi-input', () => {
xtest('create multi-input', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@greg-a-smith: why are these test being x-ed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a to-do comment stating why snapshots were failing and the assertion was commented out causing the no-unused-vars lint error. To resolve all of the lint errors, the entire test ended up being commented out so I decided to uncomment everything and just x the tests until the to-do comments are resolved.

productSwitcher={productSwitcher}
productSwitcherList={productSwitcherList} />
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Were these made with the intention of using them in the snapshot tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. There were a bunch of variables defined to create it, but then it was never used. I will check the blame on this to see if someone wants to retain all this code in a separate branch, but it should all be removed from master.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, i think i forgot to make the snapshot call. I will create a chore: PR to add the call in there. This snapshot call bumps up the code coverage from 30% to about 85%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrismanciero Do you want to fix it in this PR or create a separate one?

@greg-a-smith greg-a-smith merged commit 470deef into master Jan 15, 2019
@greg-a-smith greg-a-smith deleted the fix/eslint-no-unused-vars branch January 15, 2019 21:43
greg-a-smith added a commit that referenced this pull request Mar 5, 2019
* Enabled no-unused-vars lint rule and fixed lint errors

* solve coPilotShell no used var issue

* update files for eslint failure
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

6 participants