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

Make middleware comply with the expected shape #32

Conversation

mrcasals
Copy link
Contributor

This PR changes the Mux.Middleware.SimplifyResponse to comply with the expected shape of 2-element tuples. This PR closes #29.

Base idea is taken from elixir-tesla/tesla#483 (comment)

Please, feel free to change the name of the fields in the middleware!

If this gets merged, this might require a major version bump because it changes the expected API of the library.

@mrcasals
Copy link
Contributor Author

mrcasals commented Jul 28, 2021

For some reason, PRs #31 and this one are not running the tests on CI, could you check it out please? @mmcc @dylanjha maybe?

Tests pass locally, but it's better if we could run them on the cI!

@mrcasals mrcasals force-pushed the fix/ensure-middleware-complies-with-shape branch from d346bb3 to 32a16dc Compare July 29, 2021 06:29
@mrcasals
Copy link
Contributor Author

I've rebased this PR so that we can use GitHub Actions here and ensure the CI is green. See #33 for more info on that.

@mupkoo
Copy link
Contributor

mupkoo commented Jul 30, 2021

I created a PR with a different solution to the problem, which is using the same API as the current version. Feel free to take a look #34

@mrcasals
Copy link
Contributor Author

@mupkoo you approach looks good to me too, I'll leave it to the maintainers to decide what they prefer to merge!

@mrcasals
Copy link
Contributor Author

Actually, I'd probably go with @mupkoo's approach. The idea to keep the middleware compatible with the rest of Tesla's middlewares (as in my PR) might not be actually useful. Wrapping the Tesla public API into a Facade pattern and interacting with it seems to me like a more logical approach, since it gives a better encapsulation and removes the need of using the middleware (which took me a while to figure out because middlewares are not obvious pieces of software).

So I'm closing this PR so that efforts can be focused on @mupkoo's #34.

Thanks!

@mrcasals mrcasals closed this Jul 30, 2021
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 reports pattern_match error using Mux.Video.Uploads.create/2
2 participants