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

Warn on missing :on in joins #4074

Merged
merged 6 commits into from Jan 8, 2023

Conversation

sabiwara
Copy link
Contributor

@sabiwara sabiwara commented Jan 7, 2023

Add a warning when :on is missing and defaults to true, as proposed here:

I think emitting a warning if the on option is missing and there is no assoc is a good compromise.

Comment on lines 244 to 246
Logger.warning(
"#{join[:file]}:#{join[:line]} Missing `:on` in join#{maybe_source}, defaulting to `on: true`."
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I struggled a bit to come up with a useful warning with the proper line number, else it would be quite un-helpful:

Screenshot 2023-01-07 at 9 31 58

With IO.warn (stacktrace isn't helpful):
Screenshot 2023-01-07 at 9 55 50

@greg-rychlewski
Copy link
Member

I think documenting that it defaults to on: true would also be helpful.

Also not sure I understand why on: true had to be added to all those tests.

@sabiwara
Copy link
Contributor Author

sabiwara commented Jan 7, 2023

I think documenting that it defaults to on: true would also be helpful.

Got it, will add it 👍

Also not sure I understand why on: true had to be added to all those tests.

This was to avoid having a wall of warnings when running tests, but I can revert test files if you prefer?
I should have asked before fixing all of them, this was most of the work to be honest 😂

@greg-rychlewski
Copy link
Member

This was to avoid having a wall of warnings when running tests, but I can revert test files if you prefer?

Ah I thought the tests were run on :error level. Sorry about that.

@greg-rychlewski greg-rychlewski merged commit c5f52d6 into elixir-ecto:master Jan 8, 2023
@greg-rychlewski
Copy link
Member

Thank you!

@sabiwara sabiwara deleted the warn-join-missing-on branch January 8, 2023 13:59
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.

None yet

3 participants