Reach 100% coverage #31
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nager when we have (and are testing with) a perfectly good implementation already.
Fix WithRepr when passed a default repr function. Make the generated repr function handle more errors and provide the full class name.
…tastructers.py Note that AbstractDynamicObjectIO had an uncovered branch in updateFromExternalObject where it wanted to set the creator property, but it had the internal and external field names confused. (It tried to get and set the external id on the internal object.) That was fixed.
- Remove weird legacy behaviour that caught a POSError coming from a *module*. How on Earth...? - Simplify implementation.
The implementation was dramatically simplified and sped up. The previous complicated behaviour of manually checking sys.modules is gone, replaced with just resolve. And the hiding of import errors is gone, since that indicates a programming error that needs to be fixed.
…d_object_factory_finder
Note that the corner case of {'Class': ''} which could previously find
a default adapter or utility is no longer supported. There was a
lurking bug here if it was {'Class': None}. We shouldn't have any such
adapter or utility anyway.
Speed up the __external_resolvers__ branch for instance functions by a lot.
…precate the hook functions.
…s need to be no-cover
|
OK, we've hit 100% coverage (line, not branch; I did best-effort with branch coverage for now). Ready for serious review. |
papachoco
reviewed
Sep 22, 2017
papachoco
approved these changes
Sep 22, 2017
Separate issue, separate PR my man. It's a best practice to try to keep commits and PRs as focused on a single issue as possible. Or, as ESR says "To do large code changes correctly, factor them into a series of smaller steps such that each revision has a well-defined and provable relationship to the last." (That's one reason that I don't like squashing commits on merge unless they are trivial.) |
jamadden
commented
Sep 22, 2017
| @@ -417,8 +416,8 @@ def choose_field(result, self, ext_name, | |||
|
|
|||
| if value is not None: | |||
| # If the creator is the system user, catch it here | |||
| if ext_name is StandardExternalFields_CREATOR and is_system_user(value): | |||
| value = to_unicode(SYSTEM_USER_NAME) | |||
| if ext_name == StandardExternalFields_CREATOR and is_system_user(value): | |||
There was a problem hiding this comment.
'is' with strings is tricky on PyPy. They don't guarantee identity in the same way CPython does.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16.
Opening now so reviewers can start reviewing. There's still one function in internalization.py to complete.
There are a bunch of individual commits here. I try to keep it to one file per commit (roughly). It may be easier to review that way. There are very few semantic changes, but there are a very few corner cases that are handled differently...I don't expect them to be any issue. When there was one like that, I tried to call it out in the commit note (look for the ellipsis).