-
Notifications
You must be signed in to change notification settings - Fork 24
Include erb files for analysis, following Flay #81
Conversation
lib/cc/engine/analyzers/ruby/main.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 The nodoc
comment is extraneous since don't use rdoc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks
e1d1983
to
a7fabc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT think about extracting all of this into a Parser
class like the other languages have? Keeping that logic here made sense when it was one-liner, but I think this is complex enough that extracting makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually prefer the single file because I think the main
class is fairly small (< 5 methods), and adding encapsulating parser files might be adding needless encapsulation.
Let me know if you feel strongly though - and I can make a change.
a7fabc3
to
b392671
Compare
For the CC duplication issue, I considered extracting part of the method, but I think it adds more lines and complexity in this case. More readable as is. |
ping @wfleming @codeclimate/review Ready for re-review! |
lib/cc/engine/analyzers/ruby/main.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💄 extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Noticed 2 more small things. If we're going to exclude the duplication issue, maybe we should add it to the ignored fingerprints in config? Otherwise, LGTM. |
96a38d8
to
de59993
Compare
de59993
to
c8ec714
Compare
lib/cc/engine/analyzers/ruby/main.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minor pondering that just occurred to me: we use binread
above but just use read
here. I suspect this is down to file encodings. You comfortable having these be different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch, binread
sounds good to me.
I pulled the code from flay
and tested it out locally without an issue, and didn't realize the intention behind using File.binread
. Making switch
One small question, but LGTM. |
This erb-processing code is lifted directly from flay: https://github.com/seattlerb/flay/blob/master/lib/flay_erb.rb#L13 Add specs
c8ec714
to
620f2dd
Compare
Include erb files for analysis, following Flay
This erb-processing code is lifted directly from flay:
https://github.com/seattlerb/flay/blob/master/lib/flay_erb.rb#L13
@codeclimate/review @noahd1