Add python duplication support #5
Conversation
|
@codeclimate/review I think this is ready for a review. |
|
Looks good enough to me to move onto to trying it out. |
| def report | ||
| flay.report(StringIO.new).each do |issue| | ||
| location = issue.locations.first | ||
| io.puts "#{new_violation(issue, location).to_json}\0" |
There was a problem hiding this comment.
where does io come from - something each(&block) provides on the report?
There was a problem hiding this comment.
Apparently all of these subclasses use the same constructor so I need out the initialize method into here.
There was a problem hiding this comment.
This turns out to be an include unfortunately. I think I'm going to make flay/reporting its own class and use it in the analyzers instead of including it.
|
Had a few questions, but this looks very nice and clean! |
|
Thanks! |
* Adds ability to transform Python -> AST -> JSON -> S-Expression for flay. * Runs `Flay#report` only once now to fix exponential issue bug. * Only grabs the first reported location from flay and stores the rest as `other_locations`. * Now returns `end` line instead of the current line for both beginning and end. * Uses `_type` in the Python script to return JSON-ified AST since `type` is used by the AST. A large portion of this code was copied from the JavaScript and Ruby analyzers as well as a private Python repo.
|
|
||
| def initialize(directory:, engine_config:) | ||
| @directory = directory | ||
| @engine_config = engine_config || {} |
There was a problem hiding this comment.
will engine_config ever be nil here?
There was a problem hiding this comment.
Not sure. I think this could be nil if a repo doesn't have a config.
BlakeWilliams
force-pushed
the
bmw-python-support
branch
2 times, most recently
from
31b0b33 to
5bd54a6
Compare
| @directory = directory | ||
| @language_strategy = language_strategy | ||
| @io = io | ||
| @flay = Flay.new(flay_options) |
There was a problem hiding this comment.
should this be responsible for instantiating Flay? Thoughts about inject it (or an adapter to it) instead?
There was a problem hiding this comment.
basically, it could be a sexp_processor that has a process(sexp) method on it, in case something later on takes the place of Flay
There was a problem hiding this comment.
looking at this now, it seems like we use mass_threshold from the strategy - possibly more coupled to flay than I'd thought originally, it seems
There was a problem hiding this comment.
Yeah, it's a bit coupled but I think it makes sense.
I'll extract it into a private method which should make it a bit better.
| end | ||
|
|
||
| def report | ||
| flay.report(StringIO.new).each do |issue| |
There was a problem hiding this comment.
what's the significance of StringIo.new here? could this get moved out to a well-named method signifying what it's used for?
There was a problem hiding this comment.
Just looked it up, Flay#report accepts an output stream so flay.report(StringIO.new) is capturing the output into that StringIO instance that we don't use instead of sending it to the default $stdout.
Flay#report returns an array of Flay::Items which we then use to generate violations.
BlakeWilliams
force-pushed
the
bmw-python-support
branch
from
5bd54a6 to
551b09e
Compare
The "main" include has been replaced by with the `Reporter` class and violation generation is now handled by the `Violation` class. Engines are now initialized with `directory` and `engine_config` and must respond to the following methods: * `run` - which returns an array of s-expressions * `mass_threshold` - returns the mass threshold for the current language based on config or a default value. Engines use the `FileList` to get the list of files which it should operate on.
BlakeWilliams
force-pushed
the
bmw-python-support
branch
from
551b09e to
781ab93
Compare
This PR adds Python duplication support and fixes a few existing bugs to get this application into a good place.
Flay#reportonly once now to fix exponential issue bug.other_locations.endline instead of the current line for both beginning and end._typein the Python script to return JSON-ified AST sincetypeis used by the AST.A lot of this code is copied over from The JavaScript analyzer and the Python analyzer (from another repo). There's a good chance to refactor a lot of this but this PR is already doing a lot so I'd prefer to do that later.