Change get_scope parameter type from FunctionType to object#186
Change get_scope parameter type from FunctionType to object#186
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
=======================================
Coverage 97.96% 97.97%
=======================================
Files 34 34
Lines 2259 2268 +9
=======================================
+ Hits 2213 2222 +9
Misses 46 46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR broadens flowrep.models.parsers.object_scope.get_scope’s type annotation so it can be used with class objects (not only FunctionType), enabling scope extraction for additional callable-like objects during parsing.
Changes:
- Remove
FunctionTypeimport and widenget_scope’s parameter type hint toobject. - Keep scope construction logic based on the module globals plus builtins.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@samwaseda I've opened a new pull request, #187, to work on those changes. Once the pull request is ready, I'll request review from you. |
…lear ValueError Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
…gitignore Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Fix AttributeError in get_scope when inspect.getmodule() returns None
liamhuber
left a comment
There was a problem hiding this comment.
Yeah! Good extension. I don't see any conceptual concern broadening the type here. As long as it works for some broader testing, I'm totally happy.
Codecov is complaining in one spot that should be easy to cover -- so easy that I'm confused why your current tests aren't covering it.
The other thing I'd like to see is some "rubber meets the road" tests of actual other callables. A builtin callable like int, a class, and a class method all come to mind, but a decent LLM might think of an edge case I'm missing.
| if module is None: | ||
| module_name = getattr(func, "__module__", None) | ||
| if module_name is not None: | ||
| module = sys.modules.get(module_name) |
There was a problem hiding this comment.
Codecov is showing that this line is not covered. I'd like to get it fully covered, but I'm a little confused because it looks to me like test_none_module_fallback_via_dunder_module should be hitting this?
|
@copilot Write a unit test to cover the line |
|
@samwaseda I've opened a new pull request, #188, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Add test coverage for sys.modules fallback path in get_scope
|
@copilot Address @liamhuber's above comment: "The other thing I'd like to see is some "rubber meets the road" tests of actual other callables. A builtin callable like int, a class, and a class method all come to mind, but a decent LLM might think of an edge case I'm missing." |
|
@samwaseda I've opened a new pull request, #189, to work on those changes. Once the pull request is ready, I'll request review from you. |
…ypes Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Co-authored-by: Liam Huber <liamhuber@greyhavensolutions.com>
Add tests for get_scope with builtin types, classes, and methods
I would like to use it for class objects as well, in which case
FunctionTypeis not broad enough.