Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab #381

vc7 · 2020-10-06T04:16:56Z

`startDate` and `dueDate` property

Make them optional

GitLab may produce null value on those two properties.

The date format

The format of dueDate has become yyyy-MM-dd and which cause this error:

ERROR: Failed to parse JSON: dataCorrupted(Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "danger", intValue: nil), CodingKeys(stringValue: "gitlab", intValue: nil), CodingKeys(stringValue: "mr", intValue: nil), CodingKeys(stringValue: "milestone", intValue: nil), CodingKeys(stringValue: "due_date", intValue: nil)], debugDescription: "Date string does not match format expected by formatter.", underlyingError: nil))

`assignees` property

The assignees array is exist in the JSON but we don't have a property for that. It is not convenient if some team need to add some rules while they are able to set multiple assignees on merge request.

Reference

You can check the response of merge request of API for more detail
- https://docs.gitlab.com/ee/api/merge_requests.html

Add tests for the formatter. Adjust the test related to the format and nullable changes.

Sources/Danger/Extensions/DateFormatterExtensions.swift

vc7 · 2020-10-06T06:53:39Z

@417-72KI Hi, thanks for viewing my pull request. Btw, I have noticed that CI failed due to this change SwiftDocOrg/swift-doc@dc1cac9#diff-37ca2dd15ca0f6b1b49e78db084ef5b9R1 , shall we pump up the the swift tools version or just keep it as it?

417-72KI · 2020-10-06T07:21:59Z

@vc7
I'm sorry but I can't judge it.
But, I think it should be keeped because of Swift 5.1 compatibility.

@f-meloni
Can I get your opinion?

f-meloni · 2020-10-06T09:03:40Z

👋
If you merge master in the branch it should solve the problem :)

vc7 · 2020-10-07T14:27:34Z

@f-meloni Hi :)
I just made the branch from the newest master so I was not able to merge the newest master to my branch.
Please let me know if I missed something

f-meloni · 2020-10-07T18:35:45Z

Let me see if I can merge it for you :)

f-meloni · 2020-10-07T18:55:36Z

My bad I though I merged fc2e5b2 to master, but is still on an open PR, sorry

f-meloni

Nice PR, thank you, I have just few minor comments :)

Sources/Danger/Extensions/DateFormatterExtensions.swift

Tests/DangerTests/DateFormatterHandlerTests.swift

- Update the Package.resoved (updated by swift build)

vc7 · 2020-10-08T03:17:04Z

(Resolved)

Thanks for the advice!

For the upgrade of the library, it works great on Xcode 12, but the KeyPath syntax seems causing build failures both on my local machine's Xcode 11.0 and couple of CI tasks. Do you have this kind of issue on your side?

And after upgrade the library, I am running into and trying to solve some unexpected test failures related to DiffParser and not able to push right now. The commit d01f6f6

The failure message is as following.

Test case testParsesDiff()

XCTAssertEqual failed: ("[@@ -0,0 +1,7 @@+<?xml version="1.0" encoding="UTF-8"?>
+<Workspace
+   version = "1.0">
+   <FileRef
+      location = "self:">
+   </FileRef>
+</Workspace>, , @@ -1,7 +0,0 @@-# frozen_string_literal: true
-
-source "https://rubygems.org"
-
-git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }
-
-gem "xcpretty-json-formatter", @@ -20,7 +20,8 @@ extension ReportSection {
     init(fromSPM spm: SPMCoverage, fileManager: FileManager) {
         titleText = nil
-        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent) } }
+        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent)
+        } }
     }
 }, @@ -1,8 +1,11 @@ import Foundation
+
+
+
+
 protocol SPMCoverageParsing {
-    static func coverage(spmCoverageFolder: String, files: [String]) throws -> Report
-}
+    static func coverage(spmCoverageFolder: String, files: [String]) throws -> Re
 enum SPMCoverageParser: SPMCoverageParsing {
     enum Errors: Error {
@@ -22,6 +25,10 @@         let coverage = try JSONDecoder().decode(SPMCoverage.self, from: data)
         let filteredCoverage = coverage.filteringFiles(notOn: files)
-        return Report(messages: [], sections: [ReportSection(fromSPM: filteredCoverage, fileManager: fileManager)])
+        if filteredCoverage.data.contains(where: { !$0.files.isEmpty }) {
+            return Report(messages: [], sections: [ReportSection(fromSPM: filteredCoverage, fileManager: fileManager)])
+        } else {
+            return Report(messages: [], sections: [])
+        }
     }
 }, @@ -3,6 +3,8 @@ protocol ShellOutExecuting {
     func execute(command: String) throws -> Data
+
+
 }
 struct ShellOutExecutor: ShellOutExecuting {]") is not equal to ("[@@ -0,0 +1,7 @@+<?xml version="1.0" encoding="UTF-8"?>
+<Workspace
+   version = "1.0">
+   <FileRef
+      location = "self:">
+   </FileRef>
+</Workspace>, , @@ -1,7 +0,0 @@-# frozen_string_literal: true
-
-source "https://rubygems.org"
-
-git_source(:github) {|repo_name| "https://github.com/#{repo_name}" }
-
-gem "xcpretty-json-formatter", @@ -20,7 +20,8 @@ extension ReportSection {
     init(fromSPM spm: SPMCoverage, fileManager: FileManager) {
         titleText = nil
-        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent) } }
+        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent)
+        } }
     }
 }
 , @@ -1,8 +1,11 @@ import Foundation
 
+
+
+
+
 protocol SPMCoverageParsing {
-    static func coverage(spmCoverageFolder: String, files: [String]) throws -> Report
-}
+    static func coverage(spmCoverageFolder: String, files: [String]) throws -> Re
 
 enum SPMCoverageParser: SPMCoverageParsing {
     enum Errors: Error {
@@ -22,6 +25,10 @@         let coverage = try JSONDecoder().decode(SPMCoverage.self, from: data)
         let filteredCoverage = coverage.filteringFiles(notOn: files)
 
-        return Report(messages: [], sections: [ReportSection(fromSPM: filteredCoverage, fileManager: fileManager)])
+        if filteredCoverage.data.contains(where: { !$0.files.isEmpty }) {
+            return Report(messages: [], sections: [ReportSection(fromSPM: filteredCoverage, fileManager: fileManager)])
+        } else {
+            return Report(messages: [], sections: [])
+        }
     }
 }, @@ -3,6 +3,8 @@ 
 protocol ShellOutExecuting {
     func execute(command: String) throws -> Data
+
+
 }
 
 struct ShellOutExecutor: ShellOutExecuting {]")

Test case testChangeTypeForModified()

XCTAssertEqual failed: ("modified(hunks: [@@ -20,7 +20,8 @@ extension ReportSection {
     init(fromSPM spm: SPMCoverage, fileManager: FileManager) {
         titleText = nil
-        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent) } }
+        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent)
+        } }
     }
 }])") is not equal to ("modified(hunks: [@@ -20,7 +20,8 @@ extension ReportSection {
     init(fromSPM spm: SPMCoverage, fileManager: FileManager) {
         titleText = nil
-        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent) } }
+        items = spm.data.flatMap { $0.files.map { ReportFile(fileName: $0.filename.deletingPrefix(fileManager.currentDirectoryPath + "/"), coverage: $0.summary.percent)
+        } }
     }
 }
 ])")

Test case testChangeTypeForRenamed()

XCTAssertEqual failed: ("renamed(oldPath: "Sources/DangerSwiftCoverage/ShellOutExecutor.swift", hunks: [@@ -3,6 +3,8 @@ protocol ShellOutExecuting {
     func execute(command: String) throws -> Data
+
+
 }
 struct ShellOutExecutor: ShellOutExecuting {])") is not equal to ("renamed(oldPath: "Sources/DangerSwiftCoverage/ShellOutExecutor.swift", hunks: [@@ -3,6 +3,8 @@ 
 protocol ShellOutExecuting {
     func execute(command: String) throws -> Data
+
+
 }
 
 struct ShellOutExecutor: ShellOutExecuting {])")

Update

~~It seems some empty lines are omitted by accident during parseHunks(_:) 's isEmpty filtering~~

let lines = changes.components(separatedBy: "\n").dropFirst().filter { !$0.isEmpty }.map(parseLine)

But still wonder why this is just came out from no where 🤔 I will keep dig into this see what I can do for this.

Please share your idea if you guys have any 😃

Update

After the investigation, I have find out that swiftformat is cleaning up the trailing whitespacing for us and that causes the test data producing empty lines and leads to the failure of the tests.

vc7 · 2020-10-08T05:18:49Z

Tests/DangerTests/GitDiffTests.swift

@@ -143,6 +143,8 @@ final class GitDiffTests: XCTestCase {
        ]))
    }

+    // swiftlint:disable all
+    // swiftformat:disable all


// swiftlint:disable trailing_whitespace // swiftformat:disable trimwhitespace

I could't get any effect by using this set of disable so I turn to use all instead in the end.

I will have a look at this separately, probably will just use a swiftformat config

yeah you are right, that might be the better way to solve this kind of issues!

417-72KI

LGTM!

waiting for @f-meloni

vc7 · 2020-10-09T06:18:57Z

No code changes at a4fd57f , two link references fixed at CHANGELOG.

f-meloni · 2020-10-09T17:43:17Z

Tests/DangerTests/GitDiffTests.swift

@@ -143,6 +143,8 @@ final class GitDiffTests: XCTestCase {
        ]))
    }

+    // swiftlint:disable all
+    // swiftformat:disable all


I will have a look at this separately, probably will just use a swiftformat config

vc7 · 2020-10-12T07:24:57Z

Many thank you guys for the reviews, I have found there are still some issues related to GitLab and I will filing pull requests later on :)

vc7 added 3 commits October 6, 2020 12:42

Change dueDate to a optional property.

279d4a1

Add test and fixture for milestone without due date.

4221d3c

Add assignees property to GitLab's MergeRequest.

0a80eb5

vc7 changed the title ~~Fix dueDate of MergeRequest and Add assignees property to GitLab~~ Fix dueDate of MergeRequest and introducing assignees property to GitLab Oct 6, 2020

vc7 added 2 commits October 6, 2020 14:27

Make startDate as optional as well.

70c3dfb

Add tests for the formatter. Adjust the test related to the format and nullable changes.

Update CHANGELOG.md

f64d713

vc7 changed the title ~~Fix dueDate of MergeRequest and introducing assignees property to GitLab~~ Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab Oct 6, 2020

vc7 added 2 commits October 6, 2020 14:44

Update CHANGELOG.md

9738b83

Make the name shorter.

dd49656

vc7 commented Oct 6, 2020

View reviewed changes

Sources/Danger/Extensions/DateFormatterExtensions.swift Outdated Show resolved Hide resolved

Throw Error instrad fatalError. Add tests for the formatter handler.

b50322f

Update swift doc

f90bf13

f-meloni reviewed Oct 7, 2020

View reviewed changes

Sources/Danger/Extensions/DateFormatterExtensions.swift Outdated Show resolved Hide resolved

Tests/DangerTests/DateFormatterHandlerTests.swift Outdated Show resolved Hide resolved

Tests/DangerTests/DateFormatterHandlerTests.swift Outdated Show resolved Hide resolved

- Enhance the error-throwable test.

d01f6f6

- Update the Package.resoved (updated by swift build)

Prevent lint and format to break the sample data.

f4cc846

vc7 commented Oct 8, 2020

View reviewed changes

vc7 added 2 commits October 8, 2020 14:24

Fix line_length violation.

0e89f5c

Prevent use KeyPath.

dba8f52

vc7 requested review from 417-72KI and f-meloni October 8, 2020 07:03

417-72KI approved these changes Oct 8, 2020

View reviewed changes

Fix link references in CHANGELOG.

a4fd57f

417-72KI mentioned this pull request Oct 9, 2020

[discussion] Exclude Test folder on SwiftLint. #382

Closed

f-meloni approved these changes Oct 9, 2020

View reviewed changes

f-meloni merged commit 340023c into danger:master Oct 9, 2020

vc7 deleted the hotfix/optional_due_date branch October 19, 2020 04:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab #381

Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab #381

vc7 commented Oct 6, 2020 •

edited

Loading

vc7 commented Oct 6, 2020

417-72KI commented Oct 6, 2020

f-meloni commented Oct 6, 2020

vc7 commented Oct 7, 2020

f-meloni commented Oct 7, 2020

f-meloni commented Oct 7, 2020

f-meloni left a comment

vc7 commented Oct 8, 2020 •

edited

Loading

vc7 Oct 8, 2020

f-meloni Oct 9, 2020

vc7 Oct 10, 2020

417-72KI left a comment

vc7 commented Oct 9, 2020

f-meloni Oct 9, 2020

vc7 commented Oct 12, 2020 •

edited

Loading

Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab #381

Fix the format and nullable of startDate and dueDate, introducing assignees property to GitLab #381

Conversation

vc7 commented Oct 6, 2020 • edited Loading

startDate and dueDate property

Make them optional

The date format

assignees property

Reference

vc7 commented Oct 6, 2020

417-72KI commented Oct 6, 2020

f-meloni commented Oct 6, 2020

vc7 commented Oct 7, 2020

f-meloni commented Oct 7, 2020

f-meloni commented Oct 7, 2020

f-meloni left a comment

Choose a reason for hiding this comment

vc7 commented Oct 8, 2020 • edited Loading

(Resolved)

Update

Update

vc7 Oct 8, 2020

Choose a reason for hiding this comment

f-meloni Oct 9, 2020

Choose a reason for hiding this comment

vc7 Oct 10, 2020

Choose a reason for hiding this comment

417-72KI left a comment

Choose a reason for hiding this comment

vc7 commented Oct 9, 2020

f-meloni Oct 9, 2020

Choose a reason for hiding this comment

vc7 commented Oct 12, 2020 • edited Loading

vc7 commented Oct 6, 2020 •

edited

Loading

`startDate` and `dueDate` property

`assignees` property

vc7 commented Oct 8, 2020 •

edited

Loading

vc7 commented Oct 12, 2020 •

edited

Loading