feat: Implement advice ordering system#704
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
kakkoyun
changed the title
e0b5191 to
12c6fb1
Compare
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]> Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Add deterministic execution order for advice across aspects using namespace and order fields. Includes OrderableAdvice interface, sorting logic, and comprehensive integration tests. Signed-off-by: Kemal Akkoyun <[email protected]>
12c6fb1 to
892f9fd
Compare
RomainMuller
left a comment
RomainMuller
left a comment
There was a problem hiding this comment.
Looks pretty good, some tactical comments on certain decisions we have to think hard and deep about before they're impossible to modify...
|
|
||
| const ( | ||
| // DefaultNamespace is used when no namespace is specified | ||
| DefaultNamespace = "default" |
There was a problem hiding this comment.
So the namespace participates in ordering -- in the sense that advice is ordered by lexicographic order of namespace, then order...
And now default occupies an arbitrary position that might be...surprising? Using an empty string would, IIUC, make non-namespaced advice consistently apply before namespaced advice... Or maybe we want them to consistently apply after all namespaced advice?
Anyhow -- I think the choice of this value is not neutral and folks may occasionally need to be able to decide if they're before or after this (or do they?). Furthermore, changing this value in the future is a potentially breaking change, so we have to ask ourselves the question now.
There was a problem hiding this comment.
It was intentional to use rather arbitrary "default" so it would be easier to add a namespace lexicographically before the default namespace. My initial thought was to give some flexibility. I need to meditate on this little further. I like the idea of empty string.
There was a problem hiding this comment.
I decided that the default namespace should come last to give control over the aspects that specify a namespace.
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Signed-off-by: Kemal Akkoyun <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
==========================================
- Coverage 65.72% 65.56% -0.16%
==========================================
Files 113 115 +2
Lines 7926 8325 +399
==========================================
+ Hits 5209 5458 +249
- Misses 2192 2328 +136
- Partials 525 539 +14
🚀 New features to boost your workflow:
|
2 participants
Add deterministic and controllable execution order for advice across aspects using namespace
and order fields.
Includes
OrderableAdviceinterface, sorting logic, andcomprehensive integration tests.
Right now, only "prepend statements" implements
OrderableAdvice.We will implement more when needed.
Signed-off-by: Kemal Akkoyun [email protected]