Include erb files for analysis, following Flay #81
Conversation
| end | ||
| end | ||
|
|
||
| class Erubis < ::Erubis::Eruby # :nodoc: |
There was a problem hiding this comment.
💄 The nodoc comment is extraneous since don't use rdoc.
ABaldwinHunter
force-pushed
the
abh-erb-files
branch
from
e1d1983 to
a7fabc3
Compare
| @@ -33,8 +34,47 @@ def overage(issue) | |||
| end | |||
|
|
|||
| def process_file(file) | |||
| RubyParser.new.process(File.binread(file), file, TIMEOUT) | |||
| if File.extname(file) == ".erb" | |||
There was a problem hiding this comment.
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.
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.
ABaldwinHunter
force-pushed
the
abh-erb-files
branch
from
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! |
| end | ||
|
|
||
| # taken from Flay, https://github.com/seattlerb/flay/blob/master/lib/flay_erb.rb | ||
|
|
|
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. |
ABaldwinHunter
force-pushed
the
abh-erb-files
branch
4 times, most recently
from
96a38d8 to
de59993
Compare
ABaldwinHunter
force-pushed
the
abh-erb-files
branch
from
de59993 to
c8ec714
Compare
| end | ||
|
|
||
| def process_erb(file) | ||
| erb = File.read(file) |
There was a problem hiding this comment.
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.
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
ABaldwinHunter
force-pushed
the
abh-erb-files
branch
from
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