-
Notifications
You must be signed in to change notification settings - Fork 4
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
Don't expand unused vars #79
Conversation
Codecov Report
@@ Coverage Diff @@
## master #79 +/- ##
===========================================
+ Coverage 3.80% 66.58% +62.77%
===========================================
Files 8 9 +1
Lines 735 790 +55
Branches 107 115 +8
===========================================
+ Hits 28 526 +498
+ Misses 707 211 -496
- Partials 0 53 +53
Continue to review full report at Codecov.
|
elif key in kwargs: | ||
return kwargs[key] | ||
elif key in self.expanded_vars: | ||
return self.expanded_vars[key] |
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.
oh -- that seems like quite an elaborate (smells like a more proper of cause) solution! ;) Since would be in the "core" of things, could you add unittests for not yet covered cases in this function?
else: | ||
assert index is not None # type: ignore[unreachable] | ||
try: | ||
sl = parse_slice(index) |
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 decided not to push on subclassing string.Formatter
whenever we were talking about enhancing the extra formatters (like for shortened commit
) since iirc it was lacking slicing, so here you are (re)implementing it?
Please add tests to cover except
clause here and common cases like [-3:]
and [start:end]
.
On a positive side -- this would open the possibility for custom formatter directives (in addition to r
) ;-) ! (I am still wondering about git describe
for commits ;))
@yarikoptic Further tests added. |
@@ -8,6 +8,7 @@ minversion = 3.3.0 | |||
deps = | |||
pytest~=6.0 | |||
pytest-cov~=2.0 | |||
pytest-mock~=3.0 |
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.
note to myself: apparently we do not have tests
extra_requires and all test requirements are set in tox
making it the only kosher way to run tests.
ok, let's proceed with this one, thank you @jwodder ! |
Fixes #78.