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
Fixes #8607 - remove unnecessary module dependency resolution #155
Conversation
I haven't reviewed the code, but this makes a lot more sense and would appear to fix the bug. |
word.downcase! | ||
word | ||
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.
Could you please add some tests for this function?
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.
Some tests added
I agree it makes more sense. My tests were successful. 👍 when the inline comment is fixed. |
I think @komidore64 should check this change as well to make sure 'hammer-cli-sam' is fine with that. |
2c35a4e
to
4f803e8
Compare
this shouldn't cause any problems with |
Cool, we're good to merge then @mbacovsky |
Thanks for review, @tstrachota and @komidore64 |
Fixes #8607 - remove unnecessary module dependency resolution
👍 |
Module dependency resolution is not necessary and should be ensured by the module. From the previous fix I left ordering of the modules and reimplemented detection of loading disables modules as module deps.