EntityPathFilter variable substitutions are now delegated to (new) ResolvedEntityPathFilter
#8543
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
… has a defined but different order. stored order is now different!
Wumpf
added
🪳 bug
Wumpf
changed the title
EntityPathFilter no longer applies variable substitutions, separated into ResolvedEntityPathFilterEntityPathFilter no variable substitutions are now delegated to (new) ResolvedEntityPathFilter
Wumpf
changed the title
EntityPathFilter no variable substitutions are now delegated to (new) ResolvedEntityPathFilterEntityPathFilter variable substitutions are now delegated to (new) ResolvedEntityPathFilter
|
Web viewer built successfully. If applicable, you should also test it:
Note: This comment is updated whenever you push a commit. |
Wumpf
force-pushed
the
andreas/separate-subsituted-and-unsubsituted-rules
branch
2 times, most recently
from
02d9db2 to
2612288
Compare
Wumpf
force-pushed
the
andreas/separate-subsituted-and-unsubsituted-rules
branch
from
2612288 to
08f721e
Compare
teh-cmc
approved these changes
Jan 6, 2025
There was a problem hiding this comment.
I gave it a very quick/shallow look. There's no way I can wrap my head around all the subtleties that all this implies, but in any case the general idea makes sense to me.
The only thing I find odd is the dataframe APIs requiring resolved filters? Can we not do that? 🤔
In any case, the faster this is in, the better -- only some soak time will tell the full story here...
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.
Related
EntityPathFilterhas undefined behavior for sorting #8530What
(Motivation see ticket)
Originally I wanted
EntityPathFilterto be a hashmap of rules. However, I realized that we still want a defined order when storing path filters into the blueprint (in order to avoid changes to it when only order changes!). Therefore, it is still a btreemap but now sorts alphabetically!Generally, I struggled a bit of where to put resolved vs unresolved entity path filter/rules, but I think the end-result here makes sense for the most part and keeps the semantics clean enough: in the viewer resolved entity paths should be used whenever working with known paths whereas unresolved are used when dealing with user inputs and when storing back to the blueprint.