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

[Refactor] no-extraneous-dependencies: use moduleVisitor #1735

Merged
merged 1 commit into from Jun 1, 2020

Conversation

adamborowski
Copy link
Contributor

Hi.
I start with simple refactoring but I already have few suggestions with code changes and tests that will make no-extraneous-dependencies more flexible for monorepo projects.

This PR is a warmup. I'd like to see if our interaction goes well.

I hope you will enjoy my contribution.
Cheers,
Adam

@coveralls
Copy link

coveralls commented Apr 26, 2020

Coverage Status

Coverage decreased (-0.05%) to 97.737% when pulling 98292ed on CloudinaryLtd:master into 515957a on benmosher:master.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

@adamborowski hi! sorry for the delay :-)

This looks great! Are there any additional test cases that would now pass, but would have failed before, that we could also add in this PR?

@adamborowski
Copy link
Contributor Author

I don't think so, this is just refactoring. I will add several PRs with tests from our forked rule that we really miss.

@ljharb
Copy link
Member

ljharb commented May 31, 2020

Thanks! I can merge it now without them, since existing tests pass, but more test cases are always appreciated, so I'll wait for those if you're OK with that :-)

@adamborowski
Copy link
Contributor Author

I think for this particular PR there is no more cases.

@ljharb
Copy link
Member

ljharb commented May 31, 2020

@adamborowski can you please check "allow edits" on the RHS of the PR? also, there's a github bug where PRs from forks that aren't your own (CloudinaryLtd vs adamborowski) won't grant the permission properly, so I'll need you to add me to that fork so I can force push to the PR branch. Thanks!

@adamborowski
Copy link
Contributor Author

Added. There is no option to allow edit on the right side

@ljharb
Copy link
Member

ljharb commented Jun 1, 2020

Interesting, maybe they "fixed" their bug by hiding the checkbox in those cases :-p Thanks, I should be able to land this shortly.

@ljharb ljharb changed the title refactor: use modueVisitor for no-extraneous-dependencies [Refactor] no-extraneous-dependencies: use moduleVisitor Jun 1, 2020
@ljharb ljharb merged commit 98292ed into import-js:master Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants