Discussion: Should we error here?

[chrismarget-j]

getPropertySetsByLabel() returns an error if no matching property sets are found.

Should it?

It occurs to me that any expectation about the result count should fall to the caller, rather than to this function.

We probably have this pattern in many of the get<things>ByLabel() functions, I just happened to notice it here because I was using this code as a template for something else.

https://github.com/Juniper/apstra-go-sdk/blob/41d1f60c196d9efdec83e1b8b0e354c93e9d9f0f/apstra/api_property_sets.go#L119C1-L125C1

[rajagopalans]

If by caller you mean the ultimate consumer of the API, then yes, if I do a GetXByLabel and label does not exist I need a NotFound class of error. Same as a 404 not found on a website.
If by caller you mean the GetXByLabel method, I can see an argument for punting these error checks (none found/multiple found etc) to that layer. The use case would them be a workflow like 1.check if something exists 2. It does not exist, create it. Even in that case, you can do it by checking for the error as opposed to checking for a null value. My opinion is that it's a wash.

[chrismarget-j]

I agree that the caller of GetPropertySetByLabel() or getPropertySetByLabel() should receive a NotFound error.

But I'm not so sure about the callers of the complimentary method which returns a slice.

"give me everything matching foo" <- an empty result is valid here, I think?

With the way we're currently using it, there's no difference because the two methods are tightly coupled.

Moving the check to here feels slightly more correct.

[rajagopalans]

getByLabel (or the public function) should return , err for none or multiple.
getByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all.

I think we share code where both "get me everything matching this" and "get me one thing matching this" where the code raises no errors for the first and does error checks for the second.

I believe specifically for property sets, you cannot create more than one with the same name.

[chrismarget-j]

getByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all.

Did you mean "getByLabels" here?

This discussion is mostly just pedantry. I'm looking at it purely from the perspective of "looking only at the function signature, does this function do what I expect?"

Thinking practically, we should probably deprecate all of the public GetByLabels methods since Apstra has been policing name collisions more aggressively with each new version. We should nudge SDK consumers away from thinking name collisions are okay.

I believe specifically for property sets, you cannot create more than one with the same name.

This is version dependent. Versions we have marked as "supported" do allow this.

[rajagopalans]

getByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all.

Did you mean "getByLabels" here?
Something like getAllXByLabel getXByLabels implies a list of labels to search against.

This discussion is mostly just pedantry. I'm looking at it purely from the perspective of "looking only at the function signature, does this function do what I expect?"

Thinking practically, we should probably deprecate all of the public GetByLabels methods since Apstra has been policing name collisions more aggressively with each new version. We should nudge SDK consumers away from thinking name collisions are okay.
Yeah. GetAllXByLabel can return a list that can be empty GetXByLabel will error out if none or more than one exist of that same label.
We are saying ByLabel, but it could hold for ByName as well.

[chrismarget-j]

I see the confusion now.

Your previous comment used the same function name twice:

getByLabel (or the public function) should return , err for none or multiple.
getByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all.

...and I added additional confusing by "fixing it" wrongly in my reply.

The missing distinction here isn't "label" vs. "labels", but "thing" vs. "things".

So, I think you originally meant this?

getThingByLabel (or the public function) should return , err for none or multiple.
getThingsByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all.

[rajagopalans]

Yes that. I missed out the s

…

On Fri, Jan 12, 2024 at 12:02 PM chrismarget-j ***@***.***> wrote: I see the confusion now. Your previous comment used the same function name twice: *getByLabel* (or the public function) should return , err for none or multiple. *getByLabel* should return [] empty list, single list or multiple in the list, with potentially no error at all. ...and I added additional confusing by "fixing it" wrongly in my reply. The missing distinction here isn't "label" vs. "labels", but "thing" vs. "things". So, I think you originally meant this? getThingByLabel (or the public function) should return , err for none or multiple. getThing*s*ByLabel should return [] empty list, single list or multiple in the list, with potentially no error at all. — Reply to this email directly, view it on GitHub <#185 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARW4WZV4PWM7HEN2FXBJV3YOFUAVAVCNFSM6AAAAABBYGCEQ6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBZGY2TEOJRHA> . You are receiving this because you were assigned.Message ID: ***@***.***>