Loki: Bug fixing on _get_procedure_item#655
Conversation
reuterbal
left a comment
reuterbal
left a comment
There was a problem hiding this comment.
I agree on the change in l. 552, that's a bug.
However, do you have a reproducer for the multiple module names? If there's erroneous behaviour right now we ought to add a matching test to ensure such behaviour cannot be reintroduced in the future.
Looking at the implementation, the candidate lookup should only consider modules from unqualified imports, and then procedures defined in these. To me it seems like this would either require the same module to be imported twice via an unqualified import, or the same procedure to be defined multiple times in the module? Happy to be proven wrong of course!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
=======================================
Coverage 96.39% 96.39%
=======================================
Files 266 266
Lines 46418 46418
=======================================
Hits 44745 44745
Misses 1673 1673
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
The issue on line 552 is trivial, since item_name is not declared at that point, we got a 'referenced before assignment' error.
Then, somehow the output from get_or_create_module_definitions_from_candidates might be a list with duplicated items, so we could get something like: 'RuntimeError: Procedure XXX defined in multiple imported modules: YYY, YYY'. So, it might be better to handle the list as a set.