Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow nil GitHubMilestone descriptions #253

Merged
merged 5 commits into from
Jun 30, 2019

Conversation

smalljd
Copy link
Contributor

@smalljd smalljd commented Jun 28, 2019

The Problem

When danger-swift is executed on a PR that has a milestone associated with it, the JSON parsing will fail if the milestone's description field is nil. JSON parsing fails with the following message:

ERROR: Failed to parse JSON: valueNotFound(Swift.String, Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "danger", intValue: nil), CodingKeys(stringValue: "github", intValue: nil), CodingKeys(stringValue: "issue", intValue: nil), CodingKeys(stringValue: "milestone", intValue: nil), CodingKeys(stringValue: "description", intValue: nil)], debugDescription: "Expected String value but found null instead.", underlyingError: nil))
ERROR: Dangerfile eval failed at Dangerfile.swift
ERROR: Could not get the results JSON file at /var/folders/md/w98btl4n7qbg0xy73lwczl38jqj51g/T/danger-response.json

Proposed Solution

Since a description is an optional field for milestones, so too should it be optional in the GitHubMilestone struct.

The JSON parser breaks if there is a milestone associated with the PR, which has a blank/nil description.
Copy link
Member

@f-meloni f-meloni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR :)
Could you please update the CHANGELOG?
Also I would suggest to add another test to avoid the regression instead of replace the one we have, what do you think?

@smalljd
Copy link
Contributor Author

smalljd commented Jun 28, 2019

Sounds good! I initially had it as a separate test, will change it back and update the changelog. Thanks for taking a look 😁

@f-meloni
Copy link
Member

Looks like danger-js is broken :(
cc. @orta

@f-meloni
Copy link
Member

If you merge master back should pass the CI now 👍

@DangerCI
Copy link

Warnings
⚠️

Sources/Danger/GitHubDSL.swift#L154 - Enum element name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L170 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L184 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L227 - Enum element name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L240 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L271 - Enum element name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L291 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L349 - Enum element name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L377 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L428 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L452 - Enum element name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L471 - Variable name should be between 3 and 40 characters long: 'id' (identifier_name)

⚠️

Sources/Danger/GitHubDSL.swift#L505 - File should contain 400 lines or less: currently contains 505 (file_length)

Generated by 🚫 Danger Swift against d79af76

@f-meloni
Copy link
Member

merge on green

@peril-staging peril-staging bot merged commit 727ff81 into danger:master Jun 30, 2019
@smalljd
Copy link
Contributor Author

smalljd commented Jul 1, 2019

Sorry for being slow on the master-merge, thanks for the help @f-meloni!

@smalljd smalljd deleted the allow_nil_milestone_descriptions branch July 1, 2019 18:03
@f-meloni
Copy link
Member

f-meloni commented Jul 6, 2019

No problem :)
Just wanted to quickly release a version, in case someone had the same problem you had :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants