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 code statistics for Javascript and CoffeeScript files to rake stats task #2270

Closed
wants to merge 2 commits into from

Conversation

nfm
Copy link

@nfm nfm commented Jul 26, 2011

First time contributor so I'm not sure if I'm 'doing it right'. Should I rebase on master and submit a second pull request?

@dmathieu
Copy link
Contributor

What about the stylesheets then ?

comment_pattern = /^\s*\/\//
elsif file_name =~ /.*\.coffee$/
comment_pattern = /^\s*#/
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a when/case would be more appropriate here (and it'd allow you to avoid repeting the same comment_pattern for rb and coffee.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean something like this?

    case file_name
    when /.*\.js$/
      comment_pattern = /^\s*\/\//
    else
      comment_pattern = /^\s*#/
    end 

Copy link

Choose a reason for hiding this comment

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

why not just use /^\s*(#|\/\/)/ as comment pattern?

Copy link
Author

Choose a reason for hiding this comment

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

That'd probably suffice. You'd match // comments in rb files which isn't correct, but that's a pretty unlikely way to start a line of code!

I made it explicit so that you get an association between file extensions and comment syntaxes, and because it's more easily extended for other file types.

@nfm
Copy link
Author

nfm commented Jul 26, 2011

I'm not sure about including stylesheets. In my mind they fall into a different category to application code and client-side code. I guess this wouldn't matter except that the aggregate stats (Code LOC, Test LOC, Code to Test Ratio) could be skewed significantly by them.

What do you think?

@nfm nfm mentioned this pull request Jan 16, 2012
@nfm
Copy link
Author

nfm commented Jan 16, 2012

@josevalim While you're tackling code stats (#4478) can you please take a look at this pull request too?

@dgilperez
Copy link
Contributor

+1

2 similar comments
@ebeigarts
Copy link
Contributor

+1

@eviltrout
Copy link

+1

@steveklabnik
Copy link
Member

Should I rebase on master and submit a second pull request?

As a general note, you can rebase whatever in your branch, and then force push, and GitHub will update the pull request properly.

@josevalim
Copy link
Contributor

I think in this case, a new pull request needs to be sent because this one targets branch 3-1-stable and pull requests should be sent for master. Thanks!!

arunagw added a commit to arunagw/rails that referenced this pull request May 31, 2012
CoffeeScript files to `rake stats` task

Orignal PR was rails#2270

Thanks to @nfm
@rafaelfranca
Copy link
Member

Closed by #6566

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.

None yet

9 participants