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

variable_naming_convention and MACROs #362

Merged
merged 1 commit into from
May 2, 2016

Conversation

elbrujohalcon
Copy link
Member

the variable_naming_convention rule complains incorrectly about MACRO names

@eric-dunham
Copy link

This only appears to be an issue when the macro is the return value of something (e.g. case, function) like:

hello(Who) ->
  who(Who).

who(undefined) ->
  undefined;
who(_) ->
  ?MODULE_STRING.

The above snippet will cause elvis rock to fail with:

  - variable_naming_convention
    - The variable "MODULE_STRING" on line 18 does not respect the format defined by the regular expression '"^([A-Z][0-9a-zA-Z]*)$"'.

@eric-dunham
Copy link

eric-dunham commented Apr 27, 2016

And I apologize, I was wrong in my above assertion, but I'm leaving it there so you know that it's not return values. The following will error on the 1-arity function, but not the 0-arity:

test() ->
  ?MODULE_STRING.

test_two(_) ->
 ?MODULE_STRING.

elbrujohalcon pushed a commit to elbrujohalcon/witchcraft that referenced this pull request Apr 28, 2016
@elbrujohalcon
Copy link
Member

@andreineculau @eric-dunham I run elvis rock with latest elvis release against this module with this elvis.config and it didn't report any warning.

What versions of Erlang/OTP and elvis are you guys using?

@elbrujohalcon
Copy link
Member

Now I see it!! I changed the regex to "^([A-Z][0-9a-zA-Z]*)$" and now it complains! I'll keep digging.

@elbrujohalcon
Copy link
Member

Turns out variable_naming_convention was not in the erl_files ruleset. 😞

@eric-dunham
Copy link

eric-dunham commented Apr 28, 2016

Thanks! For reference, I'm running Elvis 0.2.11 and Erlang/OTP 18.3:

10:08:54(-0500) ~/Development/elvis {master} $ elvis rock
Loading files...
Loading src/elvis.erl
Loading src/elvis_git.erl
Loading src/elvis_webhook.erl
Loading src/eric.erl
Loading test/elvis_SUITE.erl
Loading test/elvis_meta_SUITE.erl
Loading test/git_SUITE.erl
Loading test/xref_SUITE.erl
Applying rules...
# src/elvis.erl [OK]
# src/elvis_git.erl [OK]
# src/elvis_webhook.erl [OK]
# src/eric.erl [FAIL]
  - variable_naming_convention
    - The variable "MODULE_STRING" on line 16 does not respect the format defined by the regular expression '"^([A-Z][0-9a-zA-Z]*)$"'.
# test/elvis_SUITE.erl [OK]
# test/elvis_meta_SUITE.erl [OK]
# test/git_SUITE.erl [OK]
# test/xref_SUITE.erl [OK]
Loading files...
Applying rules...
Loading files...
Applying rules...
Loading files...
Applying rules...
10:08:56(-0500) ~/Development/elvis {master} $ elvis -v
   ______     _   
  / __/ /  __(_)__
 / _// / |/ / (_-<
/___/_/|___/_/___/
Version: 0.2.11
10:08:59(-0500) ~/Development/elvis {master} $ erl
Erlang/OTP 18 [erts-7.3] [source] [64-bit] [smp:4:4] [async-threads:10] [hipe] [kernel-poll:false]

Eshell V7.3  (abort with ^G)
1> 

@eric-dunham
Copy link

eric-dunham commented Apr 28, 2016

And since I can't drag and drop a .config file, here's my elvis.config, for reference:

[
 {
   elvis,
   [
    {config,
     [#{dirs => ["src", "test"],
        filter => "*.erl",
        rules => [
          {elvis_style, dont_repeat_yourself},
          {elvis_style, function_naming_convention, #{regex => "^([a-z]*_?)*(_SUITE)?$"}},
          {elvis_style, god_modules, #{limit => 25}},
          {elvis_style, invalid_dynamic_calls},
          {elvis_style, line_length, #{limit => 120}},
          {elvis_style, macro_module_names, #{}},
          {elvis_style, macro_names, #{}},
          {elvis_style, max_function_length, #{max_length => 50, count_comments => false, count_whitespace => false}},
          {elvis_style, max_module_length, #{max_length => 1000, count_comments => false, count_whitespace => false}},
          {elvis_style, module_naming_convention, #{regex => "^([a-z][a-z0-9]*_?)*(_SUITE)?$"}},
          {elvis_style, nesting_level, #{level => 3}},
          {elvis_style, no_behavior_info, #{}},
          {elvis_style, no_debug_call},
          {elvis_style, no_if_expression, #{}},
          {elvis_style, no_nested_try_catch, #{}},
          {elvis_style, no_spec_with_records, #{}},
          {elvis_style, no_tabs, #{}},
          {elvis_style, no_trailing_whitespace, #{ignore_empty_lines => true}},
          {elvis_style, operator_spaces, #{rules => [{right, ","}, {right, "++"}, {left, "++"}]}},
          {elvis_style, state_record_and_type, #{}},
          {elvis_style, used_ignored_variable, #{}},
          {elvis_style, variable_naming_convention, #{regex => "^([A-Z][0-9a-zA-Z]*)$"}}
        ],
        ruleset => erl_files
       },
      #{dirs => ["."],
        filter => "Makefile",
        ruleset => makefiles
       },
      #{dirs => ["."],
        filter => "rebar.config",
        ruleset => rebar_config
       },
      #{dirs => ["."],
        filter => "elvis.config",
        ruleset => elvis_config
       }
     ]
    }
   ]
 }
].

elbrujohalcon pushed a commit to inaka/elvis_core that referenced this pull request Apr 28, 2016
elbrujohalcon pushed a commit to inaka/elvis_core that referenced this pull request Apr 28, 2016
elbrujohalcon pushed a commit to inaka/elvis_core that referenced this pull request Apr 28, 2016
amilkr pushed a commit to inaka/elvis_core that referenced this pull request May 2, 2016
* [#362] [inaka/elvis#362] Adding variable naming convention to erl_files ruleset

* [#362] [inaka/elvis#362] Add test

* [#362] Remove comments that showed up when tests passed

* [#362] [Fix inaka/elvis#362] Correctly distinguish macros from variables

* [#362] Fix mixed style issue
@elbrujohalcon elbrujohalcon self-assigned this May 2, 2016
@elbrujohalcon elbrujohalcon reopened this May 2, 2016
@davecaos davecaos merged commit 2e75ed8 into master May 2, 2016
@davecaos davecaos deleted the elbrujohalcon.362.variable_naming_convention_and branch May 2, 2016 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants