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

change the Mix tasks advice to be close Getting Started.md #3826

Merged

Conversation

TORIFUKUKaiou
Copy link
Contributor

Hi!

The Ecto is great!
Thanks a lot.

I have a proposal to improve the description.

{#{inspect repo}, []}

Don't forget to add your new repo to your supervision tree
(typically in lib/#{app}/application.ex):

    {#{inspect repo}, []}

I was wondering if you could improve the description in Getting Started.md ?
Of course I see the current description is right.
So I don't mind if the pull request was rejected.

@josevalim
Copy link
Member

Thank you for the PR! I think instead we should change the Mix tasks advice to simply say #{inspect repo}, instead. WDYT?

@TORIFUKUKaiou
Copy link
Contributor Author

Thanks for the lighting fast response!

I see now.
I want to work for this, can I? :)

@josevalim
Copy link
Member

Yes, please!

@TORIFUKUKaiou TORIFUKUKaiou force-pushed the improve-doc-in-Getting-Started.md branch from de21803 to c48695b Compare January 29, 2022 11:48
@TORIFUKUKaiou TORIFUKUKaiou changed the title Improve "to add your new repo to your supervision tree" in Getting Started.md change the Mix tasks advice to be close Getting Started.md Jan 29, 2022
@TORIFUKUKaiou
Copy link
Contributor Author

TORIFUKUKaiou commented Jan 29, 2022

I force push.

1. lib/mix/tasks/ecto.gen.repo.ex

lib/mix/tasks/ecto.gen.repo.ex advice is close Getting Started.md.

(a) or (b)?
I select (b).
I think that (b) is clear where he/she for Ecto beginer adds Repo.

What do you think?

(a)
pr-3826-a

(b)
pr-3826-b


2. lib/ecto.ex

change the Ecto moduledoc to simply write MyApp.Repo, in lib/ecto.ex .


3. mix test

There are 4 failures, when I did 'mix test' on c48695b.
https://github.com/elixir-ecto/ecto/tree/883cf145f2812c8b50dc03cca2de894f2848c87a has same error, too.
I use elixir: 1.13.1-otp-24, erlang: 24.2 .
So it keeps.

  • test/ecto/query/builder/dynamic_test.exs:51
  • test/ecto/query/builder/join_test.exs:24
  • test/ecto/query/inspect_test.exs:271
  • test/ecto/query/planner_test.exs:330

Hope this helps.

@josevalim josevalim merged commit 278b6fb into elixir-ecto:master Jan 29, 2022
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

@TORIFUKUKaiou TORIFUKUKaiou deleted the improve-doc-in-Getting-Started.md branch January 29, 2022 12:15
@TORIFUKUKaiou
Copy link
Contributor Author

Thank you very much.

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