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

SlapdObject command methods return process data #393

Closed
wants to merge 4 commits into from

Conversation

azmeuk
Copy link
Contributor

@azmeuk azmeuk commented Nov 10, 2020

A simple refactoring, so the output of commands is usable.

droideck
droideck previously approved these changes Nov 10, 2020
Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very small nitpick...
Could you please use an imperative mood in the subject line? It makes the changelog much more readable.

Besides that, you have my ack!

@azmeuk
Copy link
Contributor Author

azmeuk commented Nov 10, 2020

Something like "Make SlapdObject commands return their outputs" would fit better?

@encukou
Copy link
Member

encukou commented Nov 13, 2020

Before the API is set in stone, are we sure it's the right one? Would it be better to return something like subprocess.CompletedProcess?

Could you also add the return value to the docstrings and documentation?

@azmeuk
Copy link
Contributor Author

azmeuk commented Nov 15, 2020

@encukou I did not know about CompletedProcess but this seems the cleaner way to do this!
I have fixed the patch (sorry for the force-push).

@azmeuk azmeuk changed the title SlapdObject command methods return the output SlapdObject command methods return process data Nov 15, 2020
'{!r} process failed:\n{!r}\n{!r}'.format(
args, stdout_data, stderr_data
)
'Process failed: {!r}'.format(" ".join(args))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed stdout and stderr here as they have been logged a few lines ago, and large outputs as exceptions messages are totally unpracticable.

Copy link
Contributor

@droideck droideck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks clean and improved!
You have my ack.

@azmeuk
Copy link
Contributor Author

azmeuk commented Jan 8, 2021

I included this patch on python-slapd.
#203 (comment)

@azmeuk azmeuk closed this Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants