-
Notifications
You must be signed in to change notification settings - Fork 86
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
replace llvmlite.ir instead of llvmlite.llvmpy.core #1413
Conversation
Codecov Report
|
Thank you very much for taking this on! I see that you've added the import for What's missing is the replacement of
This is a good issue to get started on because these can be string-substitutions: the arguments to these functions are the same in both interfaces, so just the fully-qualified names of the functions (i.e. with all the dots) need to be replaced. Also, I can tell you that these functions were never imported using Python's These are all the files where the old functions appear: % fgrep -rl llvmpy src/
src/awkward/_connect/_numba/builder.py
src/awkward/behaviors/string.py
src/awkward/_v2/_connect/numba/builder.py
src/awkward/_v2/behaviors/string.py Although your imports are in the right scope, I don't think they're in exactly the right places, since they should be replacing imports for
Again, string substitution is good enough because they're always fully qualified. When pre-commit changes the branch, you'll have to In fact, you've made a lot of the changes I described above while I was typing them. (I should have just waited!) |
Glad to be of help. Thank you for the detailed breakdown of the issue. I'm still learning the details of using Git and it's workflow.
and all I haven't added any new import calls. Unless something else come up, this issue should be closed? |
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.
Yes, this is perfect!
There shouldn't be any new imports, since each llvmlite.llvmpy.core
becomes a llvmlite.ir
. So, no problems there.
I've attached issue #1411 to this PR (in the "Development" box on the right), so when this PR is merged, the issue will automatically be closed.
I've approved this PR, and that means that I'll be merging it when I'm certain that you're not still working on it. It looks to me like you're done. If you just give me a thumbs-up or a short comment saying you're done, I'll press the merge button.
Thanks for contributing!
Yup its all done. And thank you for being patient with me. Hopefully I can help contribute more. |
@all-contributors please add @Ahmad-AlSubaie for code |
I've put up a pull request to add @Ahmad-AlSubaie! 🎉 |
issue #1411
This is my first PR and my first open source contribution.