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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for scoped packages in ESLint special. Close #174 #176

Merged
merged 6 commits into from
Nov 18, 2016
Merged

Add support for scoped packages in ESLint special. Close #174 #176

merged 6 commits into from
Nov 18, 2016

Conversation

goloroden
Copy link

@goloroden goloroden commented Nov 18, 2016

Hi @lijunle 馃槉

I have added support for scoped modules. I did not follow the new ESLint code, but rather cleaned up the existing one and added what was needed to make things work with scoped modules.

I hope you like it 馃槉

@codecov-io
Copy link

codecov-io commented Nov 18, 2016

Current coverage is 98.73% (diff: 100%)

Merging #176 into master will increase coverage by 0.18%

@@             master       #176   diff @@
==========================================
  Files            23         23          
  Lines           622        634    +12   
  Methods         156        160     +4   
  Messages          0          0          
  Branches        140        142     +2   
==========================================
+ Hits            613        626    +13   
+ Misses            9          8     -1   
  Partials          0          0          

Powered by Codecov. Last update a9682f1...17dae2e

@goloroden goloroden changed the title Add support for scoped packages in ESLint special. Close #174 Add support for scoped packages in ESLint special. Close depcheck/depcheck#174 Nov 18, 2016
@goloroden goloroden changed the title Add support for scoped packages in ESLint special. Close depcheck/depcheck#174 Add support for scoped packages in ESLint special. Close #174 Nov 18, 2016
@goloroden
Copy link
Author

PS: Node.js 0.12 tests pass, but running depcheck on itself doesn't work on this version of Node.js. With 4.x and 6.x, everything is fine. To be honest, I do not actually know why this is :-(

Copy link
Member

@lijunle lijunle left a comment

Choose a reason for hiding this comment

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

The code looks good to me. I left the tiny comments. I will look into the failing build.

Thank you for contributing! 馃憤

}

function isEslintConfigARelativePath(specifier) {
// return !/\w|@/.test(preset.charAt(0));
Copy link
Member

Choose a reason for hiding this comment

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

Could you please clean up this comment?

Copy link
Author

@goloroden goloroden Nov 18, 2016

Choose a reason for hiding this comment

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

Done (and sorry for having forgotten to remove this one).

function resolvePresetPackage(preset, rootDir) {
// inspired from https://github.com/eslint/eslint/blob/5b4a94e26d0ef247fe222dacab5749805d9780dd/lib/config/config-file.js#L347
Copy link
Member

Choose a reason for hiding this comment

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

Please don't remove this inspired comment. It is a clue for future updates.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

{
name: 'handle config of scoped module with full name',
content: {
extends: '@my-org/eslint-config-customized',
Copy link
Member

Choose a reason for hiding this comment

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

Could you please switch another random name here? Or code is search friendly.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@goloroden
Copy link
Author

The code looks good to me.

Thanks a lot, that's very nice feedback 馃槉

I left the tiny comments.

I updated all three sections.

I will look into the failing build.

Thanks!

@lijunle
Copy link
Member

lijunle commented Nov 18, 2016

I get the reason that node 0.12 is failing, see this log: https://travis-ci.org/depcheck/depcheck/jobs/177059242#L614

node 0.12 not support String.prototype.startsWith.

- Resolve the build failing in node 0.12.x
@lijunle lijunle merged commit 8ea13b8 into depcheck:master Nov 18, 2016
@goloroden
Copy link
Author

Awesome, thanks :-)

@goloroden goloroden deleted the patch-2 branch November 18, 2016 18:28
@lijunle
Copy link
Member

lijunle commented Nov 18, 2016

@goloroden Version 0.6.5 is out, please enjoy it! Thank you again for contributing!

@goloroden
Copy link
Author

Great news, thanks again 馃槉

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

Successfully merging this pull request may close these issues.

None yet

3 participants