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

Get Dialyxir in CI #424

Merged
merged 10 commits into from Nov 14, 2018
Merged

Conversation

mmmries
Copy link
Contributor

@mmmries mmmries commented Nov 9, 2018

This is a first pass at #183

The goal for this PR is to get a simple dialyzer setup that works pretty quickly in CI and helps to catch any typespec errors.

@coveralls
Copy link

coveralls commented Nov 9, 2018

Coverage Status

Coverage remained the same at 82.505% when pulling 3068b60 on mmmries:run_dialyzer_in_ci into 7bfaa23 on code-corps:master.

mix.exs Outdated Show resolved Hide resolved
@mmmries
Copy link
Contributor Author

mmmries commented Nov 12, 2018

I'm chipping away at these dialyzer errors bit by bit and I wanted to confirm something. There were a fews typespecs that were specifying a return value of {:ok, _} | {:error, Stripe.api_error_struct}. I did a little bit of digging through the git history and it looks like a while ago this library decided to wrap all those different error cases into a singular %Stipe.Error{} struct. Is that right? I switched these typespecs to specify {:error, %Stripe.Error{}} instead, but wanted to make sure that is accurate.

body is getting serialized before passing into this fuction
now, so it no longer accepts a map
The id is [required according to the API docs](https://stripe.com/docs/api/application_fees/retrieve#retrieve_application_fee-id)
So it seems reasonable that it would be required in the library as well
There are a few issues with the test code/test dependencies,
but we don't care about type-checking that
@mmmries
Copy link
Contributor Author

mmmries commented Nov 13, 2018

If anyone gets a few minutes to review this, the code is now running dialyzer quickly and cleanly on CI. The changes were almost entirely just changing typespecs on things. There were several types (ie String.Types.metadata() => Stripe.Types.metadata()), a couple places where the code had been changed and the typespecs just needed to be updated (ie Api.body() being defined as map() instead of iodata()), and one place where the typespecs actually pointed out an error in the code itself (Stripe.Connect.ApplicationFee.retrieve having a default id of []).

Copy link
Collaborator

@snewcomer snewcomer left a comment

Choose a reason for hiding this comment

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

Wow what an awesome PR! So many small errors caught (most likely by my own doing). Wish we had this earlier. 🙇

@snewcomer snewcomer changed the title WIP: Get Dialyxir in CI Get Dialyxir in CI Nov 14, 2018
@snewcomer snewcomer merged commit 5eba509 into beam-community:master Nov 14, 2018
@mmmries mmmries deleted the run_dialyzer_in_ci branch November 14, 2018 15:55
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