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 type definition #60

Merged
merged 4 commits into from
Jun 16, 2023
Merged

Fix type definition #60

merged 4 commits into from
Jun 16, 2023

Conversation

paulo-ferraz-oliveira
Copy link
Contributor

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

+ @type parsec_success :: {:ok, tokens, String.t(), context(), integer(), integer()}
- @type parsec_success :: {:ok, tokens, String.t(), context(), {integer(), integer()}, integer()}

as consumed by e.g. makeup_elixir or makeup_erlang.

I take this time to also introduce dialyxir to CI, which surfaces another issue (opaqueness-related) already mentioned next to stream_data.

Actions missing from this:

  • waiting for a stream_data update and subsequent import here
  • (potentially) updating nimble_parsec (as per acceptance of this pull request) [optional]

Edit: while reading the CONTRIBUTING guide I didn't quite understand how to handle the RELEASE.md part (is this required?). Regarding the CHANGELOG.md, do you prefer I do it? Or do you, prior to release?

@josevalim
Copy link
Collaborator

A PR to fix the specs is welcome but we don't plan to introduce dialyzer at the moment, thank you :)

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jun 16, 2023

Sure. I can remove that bit. Thanks.

Edit: regarding the opaqueness-related issue it also only makes sense with dialyzer in place, so that's one less thing to pursue.

@paulo-ferraz-oliveira
Copy link
Contributor Author

@josevalim, shall I wait for a nimble_parsec release to update this? Or are you good without it? Thanks.

@josevalim josevalim merged commit 8e3e7bc into elixir-makeup:master Jun 16, 2023
2 checks passed
@josevalim
Copy link
Collaborator

💚 💙 💜 💛 ❤️

@paulo-ferraz-oliveira
Copy link
Contributor Author

The goal of this update is to fix a Dialyzer -related issue with makeup_erlang and makeup_elixir (for which now I'm thinking you don't want dialyxir introduced 😄). Do you think it would make sense to tag-release it, to prevent consumers from finding a dialyxir issue.

@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the fix/type-definition branch June 16, 2023 11:19
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