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 dialyxir reported issues #125

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Fix dialyxir reported issues #125

merged 4 commits into from
Jun 16, 2023

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Jun 16, 2023

ci.yml is fixed because of GitHub Actions -reported warnings.
The format changes is also because of CI -reported issues.

There's still one thing to handle potentially:

* Potential "The pattern can never match the type." issue found by dialyxir #123

(I can wait for that to either be fixed - or I can fix it if I know how)

which is what initially lead me to wanna introduce dialyxir and fix the "issues".

@paulo-ferraz-oliveira
Copy link
Contributor Author

I've rebased, ran dialyzer on top of this new code and there's no issue. I'll move to Ready for review, but feel free to Close without merging if you think the changes aren't worth it :)

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review June 16, 2023 10:53
@paulo-ferraz-oliveira
Copy link
Contributor Author

After merge, or close, though, would it be too much to ask for a release tag so your change could be imported somewhere else, pretty please? Thanks.

@josevalim josevalim merged commit ae840c8 into dashbitco:master Jun 16, 2023
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@josevalim
Copy link
Member

@paulo-ferraz-oliveira do you need a new release? I thought the changes we made do not impact external libraries, only our own usage?

@paulo-ferraz-oliveira
Copy link
Contributor Author

The ones I made, yes, only affect internally; the ones you made, though, should impact the consumer, right? (maybe not significantly is what you meant?) In any case, they'll probably eventually get updated via mix install when there's a new version, in the future 😄

As an example, absinthe depends on {:nimble_parsec, "~> 1.2.2 or ~> 1.3.0"}, but without a release, if I do mix deps.update nimble_parsec nothing changes, right?

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the fix/dialyxir-reported-issues branch June 16, 2023 11:16
@josevalim
Copy link
Member

I don't think any affect external users but I may be wrong as Dialyzer is not my strong suit. :)

@paulo-ferraz-oliveira
Copy link
Contributor Author

I was thinking about this change of yours, not the dialyxir bits, since it seems to change behaviour, but I don't know enough about the code/consumption to understand if that's the case.

@josevalim
Copy link
Member

I don't believe it changes any user facing API or behaviour!

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.

2 participants