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

Revert "Allow numbers in literal/1 (#4562)" and add identifier/1 and constant/1 instead #4563

Merged
merged 4 commits into from
Jan 9, 2025

Conversation

greg-rychlewski
Copy link
Member

@greg-rychlewski greg-rychlewski commented Jan 9, 2025

Based on our discussions, it seems there is agreement that the old literal/1 should be deprecated and we should make two new "functions": one for identifiers and one for constant values.

There was agreement by some people for identifier/1 and constant/1. If you are not in favour of those names let me know! It won't affect the PR much.

Also, I'm not sure what is the normal deprecation procedure. I think we need to keep accepting literal/1 to not break apps? If so should we remove it from the documentation or put some notes in the documentation about it being deprecated and replaced by these 2?

Companion Ecto SQL PR: elixir-ecto/ecto_sql#652

@josevalim
Copy link
Member

For deprecation we can remove the literal and make it sure it emits a warning, a compile-time warning being best. Internally, we can rewrite literal to identifier in the AST. It is probably best if we do these two changes in the Builder, so the warning is compile-time. But yeah, other than that, we need to keep on supporting for quite some time.

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jan 9, 2025

@josevalim sorry before I merge, does the way I did the deprecation look alright to you?

@josevalim
Copy link
Member

ship it!!!!

@greg-rychlewski greg-rychlewski merged commit b02e921 into elixir-ecto:master Jan 9, 2025
3 of 6 checks passed
@greg-rychlewski greg-rychlewski deleted the literals_take_2 branch January 9, 2025 13:58
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