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

Fix Danger crashing when PR has no description #237

Merged
merged 5 commits into from
May 23, 2019
Merged

Fix Danger crashing when PR has no description #237

merged 5 commits into from
May 23, 2019

Conversation

davdroman
Copy link

Fixes #236.

@DangerCI
Copy link

DangerCI commented May 20, 2019

Warnings
⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

⚠️

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

Generated by 🚫 Danger Swift against 0bf3f5e

@f-meloni
Copy link
Member

Thank you for the PR :)

I had a quick look and this should be not nullable.
This is the DSL declaration
https://github.com/danger/danger-js/blob/9a03a1103b63ac4f26e48fa1e94a46940c6eb146/source/dsl/GitHubDSL.ts#L188

And we have many PRs with no body here and they all passed without problems, last one is #235.
Will have a look at this a little bit deeper today.

@f-meloni
Copy link
Member

f-meloni commented May 20, 2019

The only strange thing looks the Issue, that is actually what is making danger crash in your log.
@orta do you have any advice on this?
On the DSL the GitHubIssue looks having just labels https://github.com/danger/danger-js/blob/9a03a1103b63ac4f26e48fa1e94a46940c6eb146/source/dsl/GitHubDSL.ts#L120
But on the JSON DSL looks having a lot more stuff https://github.com/danger/swift/blob/master/fixtures/eidolon_609.json#L367

@orta
Copy link
Member

orta commented May 20, 2019

The issue is a bigggg object, I just didn't bother typing it but almost all uses of it are just to grab the label so I didn't type the rest

@f-meloni
Copy link
Member

f-meloni commented May 20, 2019

Ok, my suggestion then would be:

  • Let's make the body optional just on the issue, because apparently is there.
  • Let's add a specific test on the decoding to be sure we don't have a regression in the future

@davdroman
Copy link
Author

davdroman commented May 22, 2019

I’ll take care of it.

@davdroman
Copy link
Author

davdroman commented May 22, 2019

Just for ref, here's an example of an issue with a null body https://api.github.com/repos/davdroman/badonde/issues/96. I'll be testing against this PR with danger-swift pr https://github.com/davdroman/Badonde/pull/96.

@davdroman
Copy link
Author

@f-meloni sorted. I didn't add specific coverage for GitHubPR decoding even though I also made its body nullable as it is required. Let me know what you think.

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.

@davdroman Looks great!
Can you please update the CHANGELOG? After that I think we are ready to go, and I will release straight away

@davdroman davdroman changed the title Fix Danger failure when PR has no description Fix Danger crashing when PR has no description May 23, 2019
@davdroman
Copy link
Author

@f-meloni ready!

@f-meloni
Copy link
Member

merge on green

@f-meloni
Copy link
Member

I think I did it too late :D

@f-meloni f-meloni merged commit 7f72d80 into danger:master May 23, 2019
@peril-staging
Copy link
Contributor

peril-staging bot commented May 23, 2019

Thanks for the PR @davdroman.

This PR has been shipped in v1.5.5 - CHANGELOG.

@davdroman davdroman deleted the fix-danger-failure-on-prs-with-no-description branch May 23, 2019 12:43
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.

Danger fails on PRs with no body
4 participants