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

Implement docstring, missing methods, basic streaming #56

Merged
merged 39 commits into from
Oct 25, 2022

Conversation

Razikus
Copy link
Collaborator

@Razikus Razikus commented Sep 11, 2022

All methods implemented

immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated
key (bytes): Key to retrieve
offset (int): Offset of history
limit (int): Limit of history entries
sortorder (bool): Sort order of history
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add information what True and False mean here.

Choose a reason for hiding this comment

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

I've resolved this with a documentation change, but I also opened a PR against this branch with a change to the method that resolves the confusion: #59

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I commented in #59
We shouldn't introduce new parameter, but just rename first one if we really want to do it. Or just write it in the documentation

immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Show resolved Hide resolved
Copy link

@NickAnderegg NickAnderegg left a comment

Choose a reason for hiding this comment

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

Most of the outstanding discussion points have been resolved by my recent commits, but I have a few open points marked in this review that need clarification.

Comment on lines +536 to +541
Args:
cleanupPercentage (float): Indicates the percentage of the
storage space that will be scanned for unreference data. Although
this operation blocks transaction processing, choosing a small
value (e.g. ``0.1``) may not significantly hinder normal operations,
and will reduce used storage space.

Choose a reason for hiding this comment

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

This is a percentage from 0 to 1, correct? If so, then it should be phrased this way instead:

Suggested change
Args:
cleanupPercentage (float): Indicates the percentage of the
storage space that will be scanned for unreference data. Although
this operation blocks transaction processing, choosing a small
value (e.g. ``0.1``) may not significantly hinder normal operations,
and will reduce used storage space.
Args:
cleanupPercentage (float): Fractional decimal indicating the percentage of the
storage space that will be scanned for unreferenced data. Although
this operation blocks transaction processing, choosing a small
value (e.g. ``0.1``) may not significantly hinder normal operations,
and will reduce used storage space.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, this is between 0 and 100, the 0.1 value is really 0.1% (which is a sane value in this case)

immudb/client.py Outdated
key (bytes): Key to retrieve
offset (int): Offset of history
limit (int): Limit of history entries
sortorder (bool): Sort order of history

Choose a reason for hiding this comment

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

I've resolved this with a documentation change, but I also opened a PR against this branch with a change to the method that resolves the confusion: #59

Args:
user (str): username
newPassword (str): new password
oldPassword (str): old password

Choose a reason for hiding this comment

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

What is it that's only necessary for the sysadmin user?

immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Show resolved Hide resolved
@NickAnderegg NickAnderegg requested a review from byo October 14, 2022 00:12
immudb/client.py Outdated Show resolved Hide resolved
immudb/client.py Outdated Show resolved Hide resolved
Args:
user (str): username
newPassword (str): new password
oldPassword (str): old password
Copy link
Contributor

Choose a reason for hiding this comment

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

This are restrictions implied by the immudb itself.

SysAdmin can change his own password only by giving old and new password.
SysAdmin user can change password of any other user without old password.
Admin users can change password for user only created by that admin without old password.

Copy link
Contributor

@mabmayer mabmayer left a comment

Choose a reason for hiding this comment

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

LGTM

@Razikus Razikus merged commit 6af811b into master Oct 25, 2022
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.

5 participants