Skip to content

Improve README setup instructions #200

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

Merged
merged 6 commits into from
Mar 12, 2025

Conversation

mweidner037
Copy link
Collaborator

Fixes #196

With thanks to @sriedel for the detailed feedback!

@mweidner037
Copy link
Collaborator Author

I removed the applications: config since that should no longer be necessary - mix should pick up our def applications() function and start the process automatically. Also, using applications (instead of extra_applications) will turn off the automatic behavior for all modules, which is probably not what you want. https://elixirforum.com/t/why-do-we-use-application-function-in-mix-exs/23834/2

Copy link
Collaborator

@scottmessinger scottmessinger left a comment

Choose a reason for hiding this comment

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

Looks great! I have one question about the Repo.pool/0 function

adapter: Mongo.Ecto

def pool() do
Ecto.Adapter.lookup_meta(__MODULE__).pid
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mweidner037 In our code base, we do %{pid: pid} = Ecto.Repo.Registry.lookup(__MODULE__). Should we switch to this form or are they equivalent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They appear to be equivalent, though perhaps the Ecto.Adapter.lookup_meta form is preferable since that function is actually documented. I'll update it when changing the Mongo Ecto versions.

@mweidner037 mweidner037 merged commit e110197 into master Mar 12, 2025
37 of 38 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.

Please improve README documentation for 2.0.0
2 participants