Skip to content
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

SendCommands return early if commands len == 0 #134

Merged
merged 3 commits into from
Jun 10, 2023

Conversation

steiler
Copy link
Contributor

@steiler steiler commented Jun 6, 2023

fixing #133

@carlmontanari
Copy link
Contributor

👋

Thanks @steiler ! I wonder if this should actually return an error instead? Or failing that, it's probably worth making sure the default settings in the MultiResponse all make sense (as in make sense to whatever degree they can when there are no responses stored in it).

Also I didn't check yet but I assume this will be a problem in both Commands/Configs -- probably we should sort both out ya? I can do that but probably wont get to it till the weekend.

Thanks again for raising this and the pr!! 🔥

@steiler
Copy link
Contributor Author

steiler commented Jun 6, 2023

Yeah wasn't sure... An error seems odd because everything is fine it is just an empty configuration file being applied. Basically that should easily go through in my view.
Not sure what it means for everything else I leave that for you to sort out.
Thanks

@hellt
Copy link
Collaborator

hellt commented Jun 6, 2023

IMHO returning error is what I'd do. Empty config payload is a no-op which makes no sense from the use case perspective, nor it bears any meaning for testing.
Catching this early with returning a clear error prevent unnecessary rpcs from being made.

@carlmontanari
Copy link
Contributor

IMHO returning error is what I'd do

I dig it.. will add a ErrNoOp or something like that error so it's very clear what happened. Will sort it this weekend if I dont get it done earlier. Thanks as always @hellt !!

@carlmontanari
Copy link
Contributor

hey ya'll -- pushed the no-op error thing. lemme know what ya think? if everyones happy ill get that merged!

@steiler
Copy link
Contributor Author

steiler commented Jun 10, 2023

Hi Carl,
I've already worked around it, now checking the length up front.
So whatever you do, except for causing a panic is fine ;-)

@carlmontanari carlmontanari merged commit 75916b6 into scrapli:main Jun 10, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants