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

Feature/max dependencies #489

Merged
merged 5 commits into from
Sep 1, 2016

Conversation

tizmagik
Copy link
Contributor

Closes #486

I decided on naming it imports/max-dependencies since it's a bit more generic (also supports require() calls) and less redundant sounding than imports/max-imports.

import Set from 'es6-set'
import isStaticRequire from '../core/staticRequire'

const DEFAULT_MAX = 10
Copy link
Contributor Author

@tizmagik tizmagik Aug 13, 2016

Choose a reason for hiding this comment

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

I'm not quite sure how defaults are handled. I wanted it so that if you enable the rule, but don't specify {max} option (is that possible?) it would at least default to a sensible 10.

Not sure if that was the right way to go about it... thoughts?

@tizmagik tizmagik mentioned this pull request Aug 13, 2016
@coveralls
Copy link

coveralls commented Aug 13, 2016

Coverage Status

Coverage increased (+0.03%) to 97.699% when pulling 1f09704 on tizmagik:feature/max-dependencies into 90dedd7 on benmosher:master.

@tizmagik
Copy link
Contributor Author

Any feedback here @benmosher @jfmengels ?

@benmosher
Copy link
Member

Sorry for the slow feedback, I think we've both been pretty busy this week and last week. I will do my best to make time Tuesday or Thursday of next week, if @jfmengels hasn't beaten me to it (and @jfmengels: don't worry about it if you're busy too).

@benmosher
Copy link
Member

Need to add a note in the change log, but beyond that, looks good to me! Thanks! Great docs and tests!

@jfmengels, any thoughts/concerns?

@coveralls
Copy link

coveralls commented Aug 22, 2016

Coverage Status

Coverage increased (+0.09%) to 97.754% when pulling 91427de on tizmagik:feature/max-dependencies into 90dedd7 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 97.754% when pulling 91427de on tizmagik:feature/max-dependencies into 90dedd7 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 97.754% when pulling 91427de on tizmagik:feature/max-dependencies into 90dedd7 on benmosher:master.


Forbid modules to have too many dependencies (`import` or `require` statements).

This is a useful rule because a module with too many dependencies is code smell, and usually indicates the module is doing too much and/or should be broken up into smaller modules.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is A code smell

@jfmengels
Copy link
Collaborator

Sorry for blocking this!

LGTM. Just a doc typo to fix, and the PR needs a rebase.

Thanks for this @tizmagik!

@tizmagik tizmagik force-pushed the feature/max-dependencies branch from 90b1fb5 to 159034c Compare August 31, 2016 20:57
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 95.882% when pulling 159034c on tizmagik:feature/max-dependencies into 28dbed8 on benmosher:master.

@coveralls
Copy link

coveralls commented Aug 31, 2016

Coverage Status

Coverage increased (+0.03%) to 97.76% when pulling 159034c on tizmagik:feature/max-dependencies into 28dbed8 on benmosher:master.

@tizmagik
Copy link
Contributor Author

tizmagik commented Aug 31, 2016

@jfmengels rebased and ready

@jfmengels
Copy link
Collaborator

LGTM! 😄

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.

4 participants