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

Dialyzer Fixes #1237

Merged
merged 1 commit into from
Nov 23, 2017
Merged

Dialyzer Fixes #1237

merged 1 commit into from
Nov 23, 2017

Conversation

asummers
Copy link
Contributor

@asummers asummers commented Nov 23, 2017

Resolved about 10 dialyzer errors, and converted case with true/false to if/else.

Discussion: the event handlers seem to all be doing the same high level logic. Perhaps it might be better to expose a validator/0 that returns a validator module, and move the with/else logic to the caller. Haven't gone through all of them yet, but I could see that being advantageous.

Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

Overall looks good. One minor comment.

@@ -33,12 +32,12 @@ defmodule CodeCorps.GitHub.Event.PullRequest do
end
end

@spec validate_payload(map) :: {:ok, :valid}
| {:error, :unexpected_payload}
@spec validate_payload(map) :: {:ok, :valid} | {:error, :unexpected_payload}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the old spec is maybe how the formatter is doing it now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh possibly. It was inconsistent across the handlers, so I just normalized them by hand. I can change it back if you want, but considering the discussion note in the PR, I feel like this code could be moved to the caller at some point soon. Your call, though.

@joshsmith
Copy link
Contributor

Can you rebase and squash into a single commit? Then I'll approve and we can merge.

@joshsmith joshsmith merged commit f90220e into develop Nov 23, 2017
@joshsmith joshsmith deleted the fix-dialyzer-1 branch November 23, 2017 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants