Avoid compilation warnings when migrating with safety_assured

[axelson]

Hi, thanks for the library! I'm currently integrating it into a project.

Is there a way to add a @safety_assured annotation without causing a compilation warning? Currently I get warnings like this:

$ mix ecto.migrate
warning: module attribute @safety_assured was set but never used
  priv/repo/migrations/20211105021748_not_null_fields.exs:3

Although to fix this, the approach for marking safety assured might need to be changed.

Maybe instead of:

@safety_assured [:not_null_added, :column_type_changed]

It could be written as:

ExcellentMigrations.safety_assured([:not_null_added, :column_type_changed])

[Artur-Sulej]

Thank you for your interest in using the lib and letting me know about warnings! 🙂

My approach was to be non-invasive for migrations (no use ExcellentMigrations, no additional function calls, etc.). @safety_assured fitted it well as an elegant comment-like/decorator. But I guess I'll need to ditch it and find a new way to mark safety.

I also don't think it's possible to suppress these warnings. I wonder how @impl, @moduledoc, @type, etc. are free of this problem, but I guess they must be whitelisted somewhere in Elixir.

Your suggestion ExcellentMigrations.safety_assured(…) looks nice, but unfortunately it won't work if excellent_migrations are added to deps with only: [:dev, :test].

For now I came up with two ideas:

• Keyword e.g. [safety_assured: [:not_null_added, :column_type_changed]] – looks a bit similar to module attribute and doesn't produce a warning.
• Config comments (like in Credo). Additional advantage is that they could be implemented in a way giving more fine-grained control – something similar to Credo's # disable-for-next-line

[Artur-Sulej]

This is a static code analysis tool (it needs source code to perform checks). I think config comments align best with that, because such approach doesn't requite any changes in migration code.

I think I'll go with # safety_assured:<comma separated check types> format and that would be applied to the line below comment. It's not as descriptive and explicit as # excellent_migrations:safety_assured_for_next_line:raw_sql_executed, but I'd rather keep it concise and readable.

Example:

https://github.com/Artur-Sulej/excellent_migrations/blob/e11c8585c2cee229b63c8fa7e1f2ae7ca2badbf1/test/example_migrations/20191026103009_safety_assured_with_config_comments.exs#L2-L5

Although I have my preferences, I'm still wondering what's better for users:

• Namespace
  • # safety_assured:<…> or
  • # excellent_migrations:safety_assured:<…>
• Delimiter
  • # safety-assured:raw-sql-executed or
  • # safety_assured:raw_sql_executed
• Amount of control
  • # safety_assured:raw_sql_executed – implicitly applying to the next line
  • # safety_assured_for_next_line:raw_sql_executed, # safety_assured_for_this_file:raw_sql_executed, etc. – more control & explicit

@axelson Which option would you be happy to see in your code?

[axelson]

I think the annotations should start with excellent_migrations so that it is obvious what library the annotation is concerned with. If possible I would pattern it after the credo annotations: https://hexdocs.pm/credo/config_comments.html

This is especially helpful for anyone that is checking out the migrations for some random project and trying to learn/understand what they do.

So maybe it would look something like # excellent_migrations:disable-for-this-file raw_sql_executed.

[Artur-Sulej]

@axelson, you convinced me to follow empathic programming™ approach. I've just released a new version v.0.1.3 that handles config comments:

• excellent_migrations:safety-assured-for-next-line raw_sql_executed
• excellent_migrations:safety-assured-for-this-file raw_sql_executed

Info in updated docs. Module attribute @safety_assured is now deprecated.

Let me know how you find this change and if you need anything more.

[axelson]

Thanks, this is much better!