-
Notifications
You must be signed in to change notification settings - Fork 65
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
Reformatting, cleanup, and coverage #193
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #193 +/- ##
==========================================
- Coverage 98.93% 98.84% -0.10%
==========================================
Files 26 26
Lines 4052 4053 +1
Branches 575 573 -2
==========================================
- Hits 4009 4006 -3
- Misses 20 24 +4
Partials 23 23
Continue to review full report at Codecov.
|
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.
I'm +0 on a few of these, +1 on most. A few things need addressing, however, like the test_basic imports. Maybe we do a separate file that's just for importing from the public API so it's clearer?
Also, while you're at it, do a fave and # pragma: no cover
branches called out by codecov in the diff. If we're gonna touch a bunch of lines, might as well make sure they're either covered or explicitly uncovered.
Thanks!
@@ -337,7 +336,7 @@ def none_or(sub_schema): | |||
'allow None or sub_schema' | |||
return Match(Or(None, sub_schema)) | |||
|
|||
assert glom(None, none_or('abc')) == None | |||
assert glom(None, none_or('abc')) == None # noqa: E711 |
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.
assert glom(None, none_or('abc')) == None # noqa: E711 | |
assert glom(None, none_or('abc')) is None |
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.
basically, the ones related to matching have to do ==
bc you can't override is
. But this one, pretty sure could be an is
. :)
@@ -454,17 +454,19 @@ def test_check_ported_tests(): | |||
assert glom(target, M(T)) == ['1'] | |||
|
|||
failing_checks = [({'a': {'b': 1}}, {'a': ('a', 'b', Match(str))}, | |||
'''expected type str, not int'''), # TODO: bbrepr at least, maybe include path like Check did | |||
# TODO: bbrepr at least, maybe include path like Check did |
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.
# TODO: bbrepr at least, maybe include path like Check did | |
# TODO: maybe include path like Check did |
ok_target = lambda: None | ||
|
||
def ok_target(): | ||
return None |
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.
return None | |
return None # pragma: no cover |
from glom import glom, SKIP, STOP, Path, Inspect, Coalesce, CoalesceError, Val, Call, T, S, Invoke, Spec, Ref | ||
from glom import Auto, Fill, Iter, A, Vars, Val, Literal, GlomError, Pipe | ||
from glom import glom, SKIP, STOP, Inspect, Coalesce, Val, Call, T, S, Invoke, Spec, Ref | ||
from glom import Fill, Iter, Literal, Pipe |
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.
ah, these imports are to ensure we don't regress the init of the module itself, I don't think we want to remove them.
|
||
from glom import OMIT, Let, Literal # backwards compat | ||
from glom import OMIT # backwards compat |
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.
same with the backwards compat ones here. they can probably move up close to the other public ones, but keep the comment.
).star(args='args2', kwargs='kwargs') | ||
spec = (Invoke(test).star(args='args') | ||
.constants(3, b='b').specs(c='c') | ||
.star(args='args2', kwargs='kwargs')) |
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.
this is better than the one before, but i think one per line might read nice here.
No description provided.