Changed behavior of ecto.gen.repo mix task in Elixir 1.9

[Eiji7]

Hi, originally I wanted to place it in mailing lists as proposition, but found that's more like bug, because it does not work as same as before Elixir release 1.9.0.

First of all this does not produces any compile-time warning:

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

Lines 85 to 93 in c0d3906

	embed_template :config, """
	use Mix.Config
	config <%= inspect @app %>, <%= inspect @mod %>,
	database: "<%= @app %>_<%= @base %>",
	username: "user",
	password: "pass",
	hostname: "localhost"
	"""

I believe that typical beginner may not even correct this. Just btw. I would like to ask if we should add warning in Mix.Config.__using__/1 macro.

The bigger problem is here:

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

Lines 54 to 55 in c0d3906

	File.write! "config/config.exs",
	String.replace(contents, "use Mix.Config\n", config_template(opts))

If we would have import Config then repo configuration would just not be added - this part looks like a bug for me as general behavior (adding config) is changed (not working as expected).

I suggest to add check like:

Version.match? "1.9.0", Mix.Project.config()[:elixir]

and store first config line in a variable.

Is it ok to make PR for this one right now or it should be discussed on mailing lists before it?

[wojtekmach]

Thank you for starting discussion on this!

use Mix.Config

First of all this does not produces any compile-time warning
I would like to ask if we should add warning in Mix.Config.__using__/1 macro.

This is because as of v1.9 Mix.Config is soft-deprecated (no warning emitted). I don't remember the exact policy but I believe after 1 or 2 minor versions a soft-deprecations is supposed to turn into a warning.

String.replace(contents, "use Mix.Config\n", config_template(opts))

If we would have import Config then repo configuration would just not be added

Good catch!

I like your idea of pushing people towards new config. However, instead of assuming that in v1.9 they use import Config, I believe a more robust solution would be to check whether user's existing config has one or the other and make sure to preserve it.

I think it's fine to keep the discussion on the issue.

[Eiji7]

@wojtekmach

I don't remember the exact policy but I believe after 1 or 2 minor versions a soft-deprecations is supposed to turn into a warning.

Ah, in that case there was no question. 😃

I like your idea of pushing people towards new config. However, instead of assuming that in v1.9 they use import Config, I believe a more robust solution would be to check whether user's existing config has one or the other and make sure to preserve it.

Ok, that makes sense for me. Please notice that after 1 or 2 minor versions in such case we would still keep old use Mix.Config, so maybe we should merge both ideas, so how about:

Instead of match for 1.9.0 we would check 1.10.0 or 1.11.0 (depends on soft-deprecation plans). If match succeed then we would force import Config. Otherwise we would follow user config.

EDIT:
I found in documentation:

The feature is effectively deprecated by emitting warnings on usage. This is also known as hard-deprecation. In order to deprecate a feature, the proposed alternative MUST exist for AT LEAST THREE minor versions. For example, Enum.uniq/2 was soft-deprecated in favor of Enum.uniq_by/2 in Elixir v1.1. This means a deprecation warning may only be emitted by Elixir v1.4 or later.
Source: https://hexdocs.pm/elixir/compatibility-and-deprecations.html#policy

Summary:

Instead of 1.9.0 check I would like to propose 1.12.0 check. If we have an Elixir version less than 1.12.0 then we would keep user config and on 1.12.0 and later we would force import Config. Does it makes sense for you?

[wojtekmach]

I’d rather not force the choice on the user. So if they don’t have a config file and use 1.9+ we’d use the new config module, but if they do, we’d preserve their choice. I like the idea of automatically overwriting people’s code but I strongly believe it should be a separate tool (like eg go fix)