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.

Sign up for GitHub

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

Jump to bottom

[Fix #468] Add include_dirs/1 to elvis_config #93

Merged
merged 3 commits into from
Aug 1, 2017
Merged

[Fix #468] Add include_dirs/1 to elvis_config #93

merged 3 commits into from
Aug 1, 2017

Conversation

harenson
Copy link
Member

@harenson harenson commented Jul 27, 2017 •
edited
Loading

Also:

@harenson harenson added the WIP label Jul 27, 2017
elbrujohalcon
elbrujohalcon requested changes Jul 28, 2017
View reviewed changes
Copy link
Member

@elbrujohalcon elbrujohalcon left a comment

Choose a reason for hiding this comment

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

Can we add a test for this? Even if we just meck ktn_code:parse_tree to be sure that it's receiving the appropriate list of directories…

src/elvis_config.erl Outdated
-spec include_dirs(Config::config() | map()) -> [string()].
include_dirs(Config) when is_list(Config) ->
lists:flatmap(fun include_dirs/1, Config);
include_dirs(RuleGroup = #{ruleset := "erl_files"}) when is_map(RuleGroup) ->
Copy link
Member

Choose a reason for hiding this comment

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

Don't be so strict, let every group have an include_dirs option, even when it might be useless today.

src/elvis_config.erl Outdated
maps:get(include_dirs, RuleGroup, ?DEFAULT_INCLUDE_DIRS);
include_dirs(RuleGroup = #{filter := "*.erl"}) when is_map(RuleGroup) ->
maps:get(include_dirs, RuleGroup, ?DEFAULT_INCLUDE_DIRS);
include_dirs(_) ->
Copy link
Member

Choose a reason for hiding this comment

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

The default should not be []. It should be ?DEFAULT_INCLUDE_DIRS instead.

@harenson harenson force-pushed the harenson.468.misuse-of-options branch 2 times, most recently from b7273a5 to 9020a48 Compare July 31, 2017 15:59
@harenson harenson changed the title [Fix #468] Add include_dirs/1 to elvis_config (WIP) [Fix #468] Add include_dirs/1 to elvis_config Jul 31, 2017
@harenson harenson added WIP and removed WIP labels Jul 31, 2017
@harenson harenson force-pushed the harenson.468.misuse-of-options branch from 9020a48 to f6ee0f9 Compare August 1, 2017 16:36
@harenson harenson removed the WIP label Aug 1, 2017
@harenson harenson force-pushed the harenson.468.misuse-of-options branch from f6ee0f9 to da034ab Compare August 1, 2017 16:59
@elbrujohalcon elbrujohalcon merged commit 3ac0584 into master Aug 1, 2017
@harenson harenson deleted the harenson.468.misuse-of-options branch August 1, 2017 17:53
@harenson harenson mentioned this pull request Aug 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elbrujohalcon elbrujohalcon elbrujohalcon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@harenson @elbrujohalcon