Skip to content

Feature/max dependencies #489

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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! 😄

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants