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 Elixir Dynamic Supervisor #62

Conversation

QuantamHD
Copy link
Contributor

No description provided.

@derekkraan
Copy link
Owner

Hi @QuantamHD,

I don't have time to check this in detail today, but I do think we need to consider how to handle the DynamicSupervisor. IMO it should go something like this:

  1. An initial commit with only the original DynamicSupervisor (without any changes at all, not even renaming the module)
  2. Let's annotate the DynamicSupervisor with the commit hash from the Elixir github repo, so that we can easily know which patches from elixir core we need to import. (in a comment at the top of the file perhaps?)
  3. Then apply any changes that are needed in separate commits.

This is going to make it easier for me to review this PR, and also make it easier in the future to keep us up to date with the Elixir DynamicSupervisor.

Aside from that, thanks for the effort and I'll make it a priority to give you more in depth feedback soon.

Derek

@QuantamHD
Copy link
Contributor Author

Okay only thing I changed here was the name of the module just because if I didn't the modules would conflict and not compile

@QuantamHD QuantamHD changed the title WIP: Add transient processes Add Elixir Dynamic Supervisor Jan 20, 2019
@QuantamHD
Copy link
Contributor Author

I needed to update the elixir version to 1.7 as the current dynamic supervisor uses the STACKTRACE construct only available in 1.7

@derekkraan
Copy link
Owner

Hi,

I noticed you had taken the WIP off the pull request title, but I'm not sure if you're asking me to merge this already?

Cheers
Derek

@QuantamHD
Copy link
Contributor Author

I was wondering if we could create a branch off master to merge this to so that any new changes would be a little easier to see?

@derekkraan derekkraan changed the base branch from master to add_transient_processes January 21, 2019 09:40
@derekkraan
Copy link
Owner

Sounds like a good idea. I've created a branch for that, I'll merge this PR in that branch so you can continue with a fresh PR.

@derekkraan derekkraan merged commit 1d12e8d into derekkraan:add_transient_processes Jan 21, 2019
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