-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix module inspect on case insensitive filesystems #8782
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
Fix module inspect on case insensitive filesystems #8782
Conversation
Earlier this outputted error messages when loaded module that existed with different casing, ex.: iex> i Datetime Caused errors to be shown. Now this checks if there is module file defines module with the same name as the provided atom to prevent such cases.
54d3917
to
c4164c7
Compare
I am not sure how we could test that on case-sensitive systems so there are no tests. If someone have any idea then I am open to it. |
@hauleth could we write a test for this that would just do nothing on case-sensitive systems so that at least when the test is running on case-insensitive systems we are testing the correct behaviour? |
@whatyouhide I can write one like that. |
Looks great, just a test and it should be good to go. I don't think the test needs to check for the filesystem because for case sensitive ones we will never show the warning either. |
@josevalim done |
lib/iex/lib/iex/info.ex
Outdated
Code.ensure_loaded?(atom) | ||
|
||
{^atom, beam, _path} -> | ||
module_name = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using Keyword.fetch
to avoid the comment and the default 0
value?
beam_info = :beam_lib.info(beam)
case Keyword.fetch(beam_info, :module) do
{:ok, ^atom} -> true
_ -> false
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to match?/2
.
This match is equivalent to == {:ok, atom}.
--
*José Valimwww.plataformatec.com.br
<http://www.plataformatec.com.br/>Founder and Director of R&D*
|
@josevalim right, I have missed that. |
lib/iex/lib/iex/info.ex
Outdated
|
||
{^atom, beam, _path} -> | ||
info = :beam_lib.info(beam) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would kill this line ✂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally always separate variables and "main body" of the function, but this is up to you.
Co-Authored-By: hauleth <lukasz@niemier.pl>
❤️ 💚 💙 💛 💜 |
Earlier this outputted error messages when loaded module that existed
with different casing, ex.:
Caused errors to be shown. Now this checks if there is module file
defines module with the same name as the provided atom to prevent such
cases.
Close #8780