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

Add update_many/2 #2771

Closed
wants to merge 2 commits into from
Closed

Add update_many/2 #2771

wants to merge 2 commits into from

Conversation

dvic
Copy link
Contributor

@dvic dvic commented Aug 15, 2023

As discussed in #2362 (comment)

TODO

  • tests
  • deprecation warning message
  • test on a real project
  • documentation

@dvic dvic marked this pull request as ready for review August 15, 2023 21:12
@dvic
Copy link
Contributor Author

dvic commented Aug 15, 2023

@josevalim This should be now ready for review. I tried to minimize the amount of changes in this PR but render_pending_components made this really difficult (without introducing a lot of duplicate code). To be honest I was surprised once the tests passed 😅 Probably a good idea to try it out in a real project as well, haven't done that yet.

@dvic
Copy link
Contributor Author

dvic commented Aug 15, 2023

Oops, the CI is failing on the warning message. Should I do this differently? A message at runtime? Not at compile time?

edit: went for the runtime route for now

@josevalim
Copy link
Member

@dvic I have been thinking about this. Instead of passing a list of assigns and a list of sockets, maybe it makes more sense to pass a list of [{assign, socket}]?

@josevalim josevalim closed this Sep 20, 2023
@josevalim
Copy link
Member

Continuing in #2771.

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