Skip to content

Conversation

@forivall
Copy link
Contributor

@forivall forivall commented Jul 4, 2016

fixes sindresorhus/atom-linter-xo#20

This was really annoying me.

  • Fix
  • Code review
  • Unit tests

index.js Outdated
var overrides = opts.overrides;
delete opts.overrides;

var filename = path.relative(opts.cwd || process.cwd(), opts.filename);
Copy link
Member

Choose a reason for hiding this comment

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

You don't need || process.cwd(). The cwd option already defaults to that.

Copy link
Member

Choose a reason for hiding this comment

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

Why are you making the path relative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

findApplicableOverrides needs the path to be relative in order to... find the applicable overrides.

|| process.cwd() removed. Thanks. I probably only read enough of options-manager to get it working.

@sindresorhus
Copy link
Member

sindresorhus commented Aug 13, 2016

Sorry, totally forgot about this one. Generally looks good. Hard to review without tests though.

"rules": {
"import/no-extraneous-dependencies": 0
"import/no-extraneous-dependencies": 0,
"ava/no-ignored-test-files": 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to ignore this since this is a package inside of the fixtures folder anyways, and so it triggered. It probably wouldn't be worth it to fix ava/no-ignored-test-files just for this.

@sindresorhus
Copy link
Member

@jamestalmage LGTY?

@forivall
Copy link
Contributor Author

I did a bit of a refactor so that instead of grouping, and then working backwards from the group, it just merges in the applicable overrides directly.

@sindresorhus
Copy link
Member

@forivall Sorry again for slow response... Looks great! I'll merge right away if you could fix the merge conflict.

@forivall
Copy link
Contributor Author

@sindresorhus Conflicts were trivial. Updated, and pending CI!

@sindresorhus sindresorhus merged commit 040d8be into xojs:master Sep 26, 2016
@sindresorhus
Copy link
Member

Yay! Thank you @forivall :) 🍰 for you.

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.

Doesn't honor configuration overrides

2 participants