-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: Generate default pairs #710
Conversation
Codecov Report
@@ Coverage Diff @@
## master #710 +/- ##
==========================================
- Coverage 12.25% 12.20% -0.06%
==========================================
Files 22 22
Lines 1453 1459 +6
==========================================
Hits 178 178
- Misses 1268 1274 +6
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cmd/definitions/type.go
Outdated
for k := range n.defaultable { | ||
keys = append(keys, k) | ||
} | ||
sort.Strings(keys) |
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.
How about using sort.Slice
on []*Pair
instead?
cmd/definitions/type.go
Outdated
@@ -593,6 +628,35 @@ func (d *Data) ValidateNamespace(srv *Service, n *Namespace) { | |||
log.Fatalf("Operation [%s] requires Pair [%s] support, please add virtual implementation for this pair.", v.Name, ps) | |||
} | |||
} | |||
|
|||
// Check if defaultable pairs are included in functions pairs. | |||
for defaultablePairName := range n.defaultable { |
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.
Can you explain a bit more about this validate logic?
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.
The intention is to check whether the default pairs belongs to the pairs of functions under namespace. For example, if location
belongs to service.op.create
, and no function in storage
has this pair, then we can't add location
as the defaultable pair for storage
.
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 think it's no so harmful that we need to check so strictly. How about adding a new issue to record this problem and remove the check logic in this PR.
From my side, I think the best way is we maintain a pairs map in namespace
and we only need to check is this pair exist in it. But we only have srv.pairs
now.
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.
Got it.
So nice! |
ref: #704