Refactor TraitsFeature #457
Closed
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
jnm2
force-pushed
the
jnm2/refactor_traits
branch
from
d9f9001
to
3582d1a
Compare
|
@OsirisTerje What do you say—skip this for 3.10 and do the equivalent of 0261bf6? |
|
Yes, that would be awesome. About the refactoring, some of it looks good, some of it I don't understand, and some are in between :-) Let's talk a bit about it before we go further, but please get 0261bf6 in ! |
|
Haha, I don't know why I thought it was complicated before... 😄 PR is up already! #467 |
|
This got stale and may be superseded by @OsirisTerje's plans. |
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.
When I came to implement the Explicit category for #47, I found TraitsFeature a bit daunting to wrap my head around.
The addition of tracking the Explicit category would introduce even more complexity, especially because I wanted to easily switch it between a trait, a category, and a brand new property. The cached representation
List<Trait>was not very easy to tinker with, the way it was mapped back and forth to TestCases and from XmlNodes. So I did a spike.This PR introduces
TestTraitInfoas a way to isolate the logic of parsing, caching, combining, and applying of test properties from each other. I expect a performance increase due to several choices in the new implementation, but simplicity was the real goal.So, for example, this would be the entire implementation of the new Explicit category: 0261bf6
@OsirisTerje Hopefully the commit messages are good explanations. If this is too many changes, that's no problem. It's just an idea.