Skip to content

Comments

Provide guidance links in mix tasks' Logger.error output#26

Open
vanderhoop wants to merge 1 commit intoArtur-Sulej:masterfrom
vanderhoop:log-breadcrumb-to-guidance-in-mix-tasks
Open

Provide guidance links in mix tasks' Logger.error output#26
vanderhoop wants to merge 1 commit intoArtur-Sulej:masterfrom
vanderhoop:log-breadcrumb-to-guidance-in-mix-tasks

Conversation

@vanderhoop
Copy link

@vanderhoop vanderhoop commented Mar 8, 2023

We just added excellent_migrations to a large repository with many newish Elixir developers contributing to it. I noticed that the current Logger.error output can be somewhat cryptic, and it isn't clear what the next steps are or where the errors originate.

This pull request adds links to the README's 'Checks' and 'Assuring Safety' section to the error output of the mix tasks.

I haven't yet determined how to do the same for the credo check, but I'll happily add it if this first PR is welcomed.

Before

Screen Shot 2023-03-08 at 2 58 27 PM

After

Screen Shot 2023-03-08 at 3 20 26 PM

@vanderhoop vanderhoop changed the title Provide links for checks Provide guidance links in mix tasks' Logger.error output Mar 8, 2023
Comment on lines +9 to +10
* https://github.com/Artur-Sulej/excellent_migrations#checks
* https://github.com/Artur-Sulej/excellent_migrations#assuring-safety
Copy link
Author

@vanderhoop vanderhoop Mar 8, 2023

Choose a reason for hiding this comment

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

The link fragments here are inherently brittle, but if the README headings change without the appropriate fragment updates here, the user will still end-up on the README document, which is still a net UX win.

@Artur-Sulej
Copy link
Owner

Thanks for the PR and using the lib!
I like your solution, because it quickly adds value and I'll be happy to merge your PR after testing.

I was thinking about introducing something similar myself. My idea was to add README links to specific dangers that were detected. Additionally these links would point to README from a lib version that is used.

For example column_renamed check is triggered when using excellent_migrations 0.1.6 – and the message would contain this link: https://github.com/Artur-Sulej/excellent_migrations/blob/v0.1.6/README.md#renaming-a-column
With this approach users are pointed to the right place in docs and links would always work even if changed in newer versions.

After adjusting links to match check names, links could be built with this logic:

def build_link(current_version, anchor) do
  anchor =
    check
    |> Atom.to_astring()
    |> String.replace("_", "-")

  "#{@base_url}/blob/v#{current_version}/README.md##{anchor}"
end

Anyway, that could be done in the future.

@vanderhoop
Copy link
Author

vanderhoop commented Mar 12, 2023

After adjusting links to match check names, links could be built with this logic:

def build_link(current_version, anchor) do
  anchor =
    check
    |> Atom.to_astring()
    |> String.replace("_", "-")

  "#{@base_url}/blob/v#{current_version}/README.md##{anchor}"
end

I actually started down a path of trying to build out links and aborted due to not knowing if you'd be interested in a bigger change, but I hadn't considered tying the links to the particular release tag to avoid brittleness. That's 💯

Happy to take on the subsequent change once this first one goes in.

@vanderhoop
Copy link
Author

@Artur-Sulej I'm curious how you're feeling about this. I just got around to auditing my dependencies on a side project, and realized my fork was behind origin. These warnings have aided me personally :)

@rafal0p
Copy link

rafal0p commented Sep 11, 2025

@Artur-Sulej , could we revive this? I came here to raise a very similar PR after trying to figure out where is this Logger.error that prevents me from merging risky migration. Is it from ecto? Is it from some library? Which library? How to express I accept the risk and would like to proceed?

Since the message is built dynamically searching for "Index not concurrently in" in deps gives you nothing. Any mention it's from excellent_migrations would be greatly appreciated :)

btw, how's life? we haven't catch up for a long time!

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.

3 participants