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: swap the parameters in Ash.ToTenant.to_tenant/2 #1003

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

rbino
Copy link
Contributor

@rbino rbino commented Apr 10, 2024

This is technically a breaking change, but without this the protocol would never work as intended since the funtion gets dispatched based on the type of the first parameter and the resource parameter will always be an atom (precisely, an alias) so the implementation for Atom will always be called

I'll add regression tests in another PR where I call the protocol in actions, I've wanted to leave this separate to highlight the need for a breaking change

Contributor checklist

  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

Sorry, something went wrong.

This is technically a breaking change, but without this the protocol would never
work as intended since the funtion gets dispatched based on the type of the
first parameter and the resource parameter will always be an atom (precisely, an
alias) so the implementation for Atom will always be called
@rbino
Copy link
Contributor Author

rbino commented Apr 10, 2024

I've realized that we could work around that in a non-breaking way if we define a new function on the protocol (e.g. to_tenant_id, or some other name, naming things is hard etc etc) with the parameters in the correct order and make the Atom implementation of to_tenant call that with swapped arguments while printing some kind of deprecation warning.

Should we go down this road to be extra careful?

@rbino
Copy link
Contributor Author

rbino commented Apr 10, 2024

Actually, I guess that this can be considered a non breaking change, since no one can be relying on the current behavior because the call will never be dispatched to custom defimpls in the current status

@zachdaniel zachdaniel merged commit 8eb98bc into ash-project:main Apr 10, 2024
30 checks passed
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