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

Resolve Dialyzer errors on Elixir 1.16 #643

Merged
merged 17 commits into from
Jun 19, 2024

Conversation

nathany-copia
Copy link
Contributor

@nathany-copia nathany-copia commented Dec 27, 2023

closes #642

This is to address this Dialyzer error seen in Elixir 1.16.

Total errors: 3, Skipped: 0, Unnecessary Skips: 0
done in 0m1.17s
lib/tesla/multipart.ex:94: Function add_file/2 has no local return
lib/tesla/multipart.ex:94: Function add_file/3 has no local return
lib/tesla/multipart.ex:106: The call 'Elixir.File':'stream!'(_path@1::binary() | maybe_improper_list(binary() | maybe_improper_list(any(),binary() | []) | char(),binary() | []),[],2048) breaks the contract ('Elixir.Path':t(),'line' | pos_integer(),[stream_mode()]) -> 'Elixir.File.Stream':t()
done (warnings were emitted)
Halting VM with exit status 2

Dialyzer is also added to CI as a separate job (it can be rather slow, and we may want to control success/failure independently for Dialyzer)

mix.exs Show resolved Hide resolved
@nathany-copia
Copy link
Contributor Author

nathany-copia commented Dec 27, 2023

FYI, I'm seeing a number of test failures (8) locally -- on the master branch as well.

mix.exs Outdated Show resolved Hide resolved
lib/tesla/multipart.ex Outdated Show resolved Hide resolved
@nathany-copia
Copy link
Contributor Author

@yordis Are there any other changes needed before we can merge this and publish a Tesla release that supports Elixir 1.16?

@yordis
Copy link
Member

yordis commented Jan 19, 2024

We need to add OTP 26 to the CI pipeline to make sure it is green

@nathany-copia
Copy link
Contributor Author

CI updated.

I don't think we need any matrix exceptions based on the versions I saw over here, but I'm happy to make other changes.
https://github.com/ex-aws/ex_aws_s3/pull/239/files

Thanks.

@peaceful-james
Copy link
Contributor

I am looking forward to this being merged :)

@nathany-copia
Copy link
Contributor Author

Merged in master.

@yordis
Copy link
Member

yordis commented Jun 18, 2024

@nathany-copia could you use #623 as the baseline? Really appreciate your support here.

Related to edgurgel/httparrot#38

The fix is in httparrot if I am not mistaken

# Conflicts:
#	.github/workflows/release.yml
#	.github/workflows/test.yml
@nathany-copia
Copy link
Contributor Author

nathany-copia commented Jun 19, 2024

@yordis With Dialyzer passing on all Elixir versions, I hope we can review and merge this even without the httparrot issues fixed, as that would allow us to stop using a fork of Tesla.

@nathany-copia
Copy link
Contributor Author

Please let me know if you'd like any other changes.

Thanks!

Copy link
Member

@yordis yordis left a comment

Choose a reason for hiding this comment

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

Really appreciate your support, thank you so much 🚀 💜

@yordis yordis merged commit 34a5108 into elixir-tesla:master Jun 19, 2024
3 of 5 checks passed
@yordis
Copy link
Member

yordis commented Jun 19, 2024

Hopefully we can figure out the elixir 1.6+ situation next 😭

@nathany-copia
Copy link
Contributor Author

Hopefully we can figure out the elixir 1.6+ situation next 😭

Absolutely. I already opened a pull request to httparrot, so we'll see how that goes.

@yordis We'd appreciate another hex release if you don't mind. ❤️

@peaceful-james I'll leave this branch here for a bit, just in case you're using it.

@peaceful-james
Copy link
Contributor

@nathany-copia I am not using it but thank you.

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.

Dialyzer failing on Tesla (master)
3 participants