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

Add an option to skip comments when checking line length rule #207

Merged
merged 4 commits into from
Mar 4, 2015
Merged

Add an option to skip comments when checking line length rule #207

merged 4 commits into from
Mar 4, 2015

Conversation

tyler-eon
Copy link
Contributor

  • New config option for line length rule: skip_comments.
  • Added skip_comments => false default value to configs.
  • Made appropriate test changes.

Reasoning: I had a URL in one of my comments which ran over 100 characters in length. Instead of trying to split the URL somehow or getting rid of it altogether, I decided I wanted a "skip comments" option in Elvis when checking line lengths. After some consideration I decided this was indeed a useful feature as the only lines I could see myself allowing to go over a specific limit as comment lines with URLs in them.

- New config option for line length rule: `skip_comments`.
- Added `skip_comments => false` default value to configs.
- Made appropriate test changes.
@@ -304,9 +305,23 @@ result_node_line_col_fun(Msg) ->

%% Rule checking

-spec line_is_comment(binary()) -> true | false.
line_is_comment(Line) ->
case re:run(Line, "^ *%") of
Copy link
Member

Choose a reason for hiding this comment

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

I think you should use ^\s* or ^[ \t]* instead of ^ * to account for lines that start with tabs.
Besides that you're only skipping those lines that are whole comments. I assume it's on purpose, right? e.g. the following line will result in a warning even if options are #{limit => 60, skip_comments => true}.

  Timeout = 7 * 24 * 60 * 60 * 1000, % A week in milliseconds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only looking at whole-line comments was, yes, a purposeful decision. It may just be a personal thing for me, but if we wanted to give more power to the rule we could go from accepting a boolean value to something like:

skip_comments = false | whole_line | any

Then for the any value we would just use a regex to remove line-end comments before evaluating length. Let me know if you'd like to see that now or not.

Copy link
Member

Choose a reason for hiding this comment

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

I like that option, I would go with skip_comments :: none | whole_line | any
and don't forget to document your changes in the wiki

Add additional options to consider end-of-line comments in addition to
whole-line comments. The new options are:

    skip_comments = false | whole_line | any.

Where `false` means never skip comments of any kind when checking line
length, and `whole_line` only ignores whole-line comments, and `any` will
skip both whole-line comments and end-of-line comments.
elbrujohalcon pushed a commit that referenced this pull request Mar 4, 2015
Add an option to skip comments when checking line length rule
@elbrujohalcon elbrujohalcon merged commit 7ec26f7 into inaka:master Mar 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants