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

Make separate constructors for RO/RW sessions #101

Merged
merged 2 commits into from
Sep 7, 2022

Conversation

ionut-arm
Copy link
Member

The previous constructor took a boolean argument which made it difficult
to understand what the user code would be doing. That constructor is now
two separate functions, one for RO sessions, one for RW sessions. The
_no_callback part of the method name was also removed and added to the
documentation instead.

cc @ximon18

The previous constructor took a boolean argument which made it difficult
to understand what the user code would be doing. That constructor is now
two separate functions, one for RO sessions, one for RW sessions. The
`_no_callback` part of the method name was also removed and added to the
documentation instead.

Signed-off-by: Ionut Mihalcea <[email protected]>
ro_session.login(UserType::User, Some(USER_PIN))?;

// generate a key pair
// This should NOT work using the Read/Write session
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant "Read-Only" in this comment rather than "Read/Write".

@ximon18
Copy link
Contributor

ximon18 commented Sep 5, 2022

The _no_callback part of the method name was also removed and added to the
documentation instead.

Should the same change also be made to session_management::open_session_no_callback() for consistency?

@ionut-arm
Copy link
Member Author

Should the same change also be made to session_management::open_session_no_callback() for consistency?

I was wondering, but I felt more comfortable with a more verbose (but accurate) name for an internal function, though maybe consistency is more important 🤔 Will change

Signed-off-by: Ionut Mihalcea <[email protected]>
Copy link

@mohamedasaker-arm mohamedasaker-arm left a comment

Choose a reason for hiding this comment

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

Thanks ✅

Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

This looks good indeed 👍 just remember to bump the minor version (since the API change) but this is a nice improvement for readability.

@ionut-arm ionut-arm merged commit badc344 into parallaxsecond:main Sep 7, 2022
@ionut-arm ionut-arm deleted the ro-rw-sessions branch September 7, 2022 08:45
@ionut-arm ionut-arm added the breaking-change Marker for PRs that break the interface of one of the crates label Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Marker for PRs that break the interface of one of the crates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants