-
Notifications
You must be signed in to change notification settings - Fork 5
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
[REF] Refactor utilities #385
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #385 +/- ##
==========================================
- Coverage 98.37% 98.36% -0.01%
==========================================
Files 16 21 +5
Lines 982 980 -2
==========================================
- Hits 966 964 -2
Misses 16 16 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Pull Request Test Coverage Report for Build 11784441718Details
💛 - Coveralls |
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.
Thanks @alyssadai for this refactor. I think it looks very nice!
I have some very minor clarity comments, take a look.
remove test test_generate_context() (it replicates logic of function being tested and is hard to maintain)
agreed, but I think we already had the case that we didn't include a class in the context and that then breaks things in confusing ways. So I don't think removing the test completely is a solution either. I don't have a great idea, but let's discuss this in a follow-up issue.
🧑🍳
Hey @surchs, just to reply to your concern regarding the removed test:
To clarify, we added two new tests in PR #349, |
🚀 PR was released in |
Changes proposed in this pull request:
utilities
moduleutility.py
tomodel_utils.py
(includes utility functions related to generating + extracting entities from graph dataset instances, etc.)test_utility.py
into:test_putil.py
test_butil.py
test_dutil.py
test_futil.py
test_mutil.py
integration/
andunit/
subdirstest_generate_context()
(it replicates logic of function being tested and is hard to maintain)Checklist
This section is for the PR reviewer
[ENH]
,[FIX]
,[REF]
,[TST]
,[CI]
,[MNT]
,[INF]
,[MODEL]
,[DOC]
) (see our Contributing Guidelines for more info)skip-release
(to be applied by maintainers only)Closes #XXXX
For new features:
For bug fixes: