Handle reporters with absolute path as output path #49

mihai8804858 · 2016-12-21T21:09:57Z

This PR will fix #48 issue.

Also, slightly refactored reporter coordinators.

codecov-io · 2016-12-21T21:52:22Z

Current coverage is 93.10% (diff: 100%)

Merging #49 into master will increase coverage by 0.34%

@@             master        #49   diff @@
==========================================
  Files            61         61          
  Lines          1657       1683    +26   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1537       1567    +30   
+ Misses          120        116     -4   
  Partials          0          0

Powered by Codecov. Last update ff5d8a6...f788f8a

lexorus

The mentioned issue itself is fixed.

lexorus · 2016-12-22T12:39:45Z

TaylorIntegrationTests/ReporterComparator.swift

@@ -33,14 +33,14 @@ func !=(lhs: [String:AnyObject], rhs: [String:AnyObject]) -> Bool {

 class ReporterComparator {

-    func compareReporters(_ firstReporterPath: String, secondReporterPath: String) -> Bool {
-        var violations1 = JSONToViolationParser().parseFile(firstReporterPath)
+    func compareReporter(atPath path1: String, withReporterAtPath path2: String) -> Bool {


This is the function that gets two paths, then parses this files to get violations, then compares each violation through iteration.
All this is happening in compareReporter(atPath:withReporterAtPath:) function of ReporterComparator class.
The Reporter word leads to a delusion here(we also have reporters in this project).
Can you think about a better name, like compareViolations, or for a better place/implementation of this functionality(extension)?

This class takes 2 paths to reports and compare them. I'm not interested how it compares the reports, it's his problem. Also, If I would see compareViolations function I would think that I should give it an array of Violation.

Then, maybe will be better to name it compareReport(atPath:withReportAtPath:) (report and not reporter)?

Already done that.

lexorus · 2016-12-22T12:45:53Z

TaylorFramework/Modules/temper/Output/OutputCoordinator.swift

        }
    }
+
+    private func writeOutput(with violations: [Violation], reporter: Reporter) {


I think we can get rid of the Output word here.

lexorus · 2016-12-22T13:04:53Z

Also, there is a Typo in commit:
Handle absolute paths when specifie reporter's output path in arguments

thelvis4 · 2016-12-22T19:18:04Z

@mihai8804858 Tests are failing 😢

mihai8804858 · 2016-12-22T21:28:41Z

@thelvis4 Don't know why 😢 Have you tried to run the tests on your machine? On my machine and on @lexorus's the tests are passing.

thelvis4 · 2016-12-27T11:50:38Z

@mihai8804858 @lexorus Checks are failing because code coverage decreased. 😢

thelvis4 · 2016-12-27T13:19:53Z

TaylorFramework/Modules/temper/Output/PMDCoordinator.swift

+    private func addElements(to xml: XMLDocument, from violations: [Violation]) {
+        for path in filePaths(from: violations) {
+            guard let fileElement = xmlElement(withFilePath: path) else { continue }
+            violations.filter { $0.path == path }


It seems like we do a lot of iteration here. A big number of violations can lead to a computation time growth.
One way we can reduce the numbers of loops is by iterating through violations (just once), group them into a dictionary that uses paths as keys:

private typealias ViolationsMap = [Path: [Violation]] final class PMDCoordinator: WritingCoordinator { func writeViolations(_ violations: [Violation], atPath path: String) { FileManager().removeFileAtPath(path) let map = mapViolations(violations) let xml = generateXML(from: map) let xmlData = xml.xmlData(withOptions: Int(XMLNode.Options.nodePrettyPrint.rawValue)) do { try xmlData.write(to: URL(fileURLWithPath: path), options: .atomic) } catch let error { let message = "Error while writing XML object at path: " + path + "\n" + "Reason: " + error.localizedDescription Printer(verbosityLevel: .error).printError(message) } } private func mapViolations(_ violations: [Violation]) -> ViolationsMap { return violations.reduce([Path : [Violation]]()) { result, violation in let path = violation.path // Add violation to the group with the same path var violations = result[path] ?? [] violations.append(violation) // Update the result var nextResult = result nextResult[path] = violations return result } } private func generateXML(from violationsMap: ViolationsMap) -> XMLDocument { let xml = XMLDocument(rootElement: XMLElement(name: "pmd")) xml.version = "1.0" xml.characterEncoding = "UTF-8" let fileElements = violationsMap.map(generateFileElement) xml.rootElement()?.addChildren(fileElements) return xml } private func generateFileElement(path: Path, violations: [Violation]) -> XMLElement { let element = XMLElement(name: "file") let attribute = XMLNode(kind: .attribute) attribute.setValue(path, forKey: "name") element.addAttribute(attribute) violations.forEach { element.addChild($0.toXMLElement()) } return element } } extension XMLElement { func addChildren(_ children: [XMLElement]) { children.forEach { child in addChild(child) } } }

What do you think?

I think you did a good job! 👍

thelvis4 · 2016-12-27T13:24:44Z

TaylorIntegrationTests/TaylorIntegrationTests.swift

@@ -17,7 +17,7 @@ enum IntegrationTestsError: Error {

 class TaylorIntegrationTests: QuickSpec {

-    var runTaskPath : String {
+    var taylorExecutablePath : String {


How about executableFilePath?
If we rename the tool, we don't want to be needed to make too many changes.

thelvis4 · 2016-12-27T13:25:12Z

TaylorIntegrationTests/TaylorIntegrationTests.swift

@@ -88,6 +88,23 @@ class TaylorIntegrationTests: QuickSpec {
        }
    }

+    fileprivate func runTaylorWithArguments(_ arguments: String...) {


runWithArguments?

thelvis4 · 2016-12-27T13:26:39Z

TaylorIntegrationTests/TaylorIntegrationTests.swift

-                    }
-                } catch let error {
-                    print(error)
+            context("when specifie report with relative path") {


Typo here at specifie

thelvis4 · 2016-12-27T13:27:09Z

TaylorIntegrationTests/TaylorIntegrationTests.swift

+                }
+            }
+
+            context("when specifie report with absolute path") {


Same tipo (specifie) here 😉

Typo here: tipo -> typo 😉

thelvis4 · 2016-12-28T08:51:43Z

@mihai8804858 There is a typo in the commit message. Could you please fix it?

thelvis4 · 2016-12-28T08:56:43Z

TaylorFrameworkTests/TemperTests/NSFileManagerTests.swift

@@ -26,6 +36,18 @@ class NSFileManagerTests: QuickSpec {
                manager.removeFileAtPath(filePath)
                expect(manager.fileExists(atPath: filePath)).to(beFalse())
            }
+
+            it("should't remove file at path if it's not deleatable") {


There is no such word deleatable. You can use deletable or delible, but I recommend to use removable here as it's less pretentious.

Typo: should't

There is a method in 'FileManager' called 'isDeleatableFileAtPath', I throught there is such word. 🙂

mihai8804858 · 2016-12-28T10:25:39Z

@thelvis4 Fixed typos.

thelvis4 · 2016-12-28T19:15:40Z

@mihai8804858 It seems like we introduced a bug here.
Before we had

<file name="/Some/path/to/file.swift">

but now we have

<file /Some/path/to/file.swift="">

Could you please investigate and also write some tests to make sure the bug is fixed and we won't have regressions?

thelvis4 · 2016-12-28T19:17:36Z

TaylorFrameworkTests/TemperTests/NSFileManagerTests.swift

+                expect(manager.fileExists(atPath: filePath)).to(beTrue())
+                FileManager.default.removeFileAtPath(filePath)
+            }
+
            it("should't remove directory at path") {


It seems like we have the same typo here: should't

mihai8804858 · 2016-12-28T20:10:47Z

@thelvis4 Done.

thelvis4 · 2016-12-28T20:23:33Z

@mihai8804858 How about the issue with file element? It's still reproducing to me.

mihai8804858 · 2016-12-28T22:22:47Z

Oops, I missed that comment. 😕

Slightly refactored reporter coordinators

thelvis4 · 2016-12-29T09:49:49Z

@mihai8804858 Thanks for working on this! 👍
You just made Taylor better! 👧 😊

mihai8804858 assigned thelvis4 Dec 21, 2016

mihai8804858 requested a review from thelvis4 December 21, 2016 21:09

lexorus reviewed Dec 22, 2016

View reviewed changes

mihai8804858 changed the title ~~Handle absolute paths when specifie reporter's output path in arguments~~ Handle reporters with absolute path as output path Dec 22, 2016

thelvis4 reviewed Dec 27, 2016

View reviewed changes

thelvis4 suggested changes Dec 27, 2016

View reviewed changes

mihai8804858 added the bug label Dec 28, 2016

thelvis4 suggested changes Dec 28, 2016

View reviewed changes

thelvis4 approved these changes Dec 28, 2016

View reviewed changes

Handle reporters with absolute path as output path

f788f8a

Slightly refactored reporter coordinators

thelvis4 merged commit b9fee76 into yopeso:master Dec 29, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle reporters with absolute path as output path #49

Handle reporters with absolute path as output path #49

mihai8804858 commented Dec 21, 2016 •

edited

Loading

codecov-io commented Dec 21, 2016 •

edited

Loading

lexorus left a comment

lexorus Dec 22, 2016

mihai8804858 Dec 22, 2016

lexorus Dec 22, 2016 •

edited

Loading

mihai8804858 Dec 22, 2016

lexorus Dec 22, 2016

lexorus commented Dec 22, 2016

thelvis4 commented Dec 22, 2016

mihai8804858 commented Dec 22, 2016

thelvis4 commented Dec 27, 2016

thelvis4 Dec 27, 2016

mihai8804858 Dec 27, 2016

thelvis4 Dec 27, 2016

thelvis4 Dec 27, 2016

thelvis4 Dec 27, 2016

thelvis4 Dec 27, 2016

mihai8804858 Dec 27, 2016

thelvis4 Dec 27, 2016

thelvis4 commented Dec 28, 2016

thelvis4 Dec 28, 2016

thelvis4 Dec 28, 2016

mihai8804858 Dec 28, 2016

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 28, 2016

thelvis4 Dec 28, 2016

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 28, 2016

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 29, 2016

Handle reporters with absolute path as output path #49

Handle reporters with absolute path as output path #49

Conversation

mihai8804858 commented Dec 21, 2016 • edited Loading

codecov-io commented Dec 21, 2016 • edited Loading

Current coverage is 93.10% (diff: 100%)

lexorus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexorus Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lexorus commented Dec 22, 2016

thelvis4 commented Dec 22, 2016

mihai8804858 commented Dec 22, 2016

thelvis4 commented Dec 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thelvis4 commented Dec 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 28, 2016

Choose a reason for hiding this comment

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 28, 2016

mihai8804858 commented Dec 28, 2016

thelvis4 commented Dec 29, 2016

mihai8804858 commented Dec 21, 2016 •

edited

Loading

codecov-io commented Dec 21, 2016 •

edited

Loading

lexorus Dec 22, 2016 •

edited

Loading