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.

Sign up for GitHub

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

Jump to bottom

remove vavr from public make methods #67

Merged
merged 1 commit into from
Aug 7, 2023
Merged

remove vavr from public make methods #67

merged 1 commit into from
Aug 7, 2023

Conversation

KannarFr
Copy link
Contributor

@KannarFr KannarFr commented Jun 2, 2023

Closes #66

@KannarFr KannarFr requested a review from Geal June 5, 2023 09:40
@KannarFr
Copy link
Contributor Author

KannarFr commented Aug 4, 2023

@divarvel @Geal

divarvel
divarvel reviewed Aug 7, 2023
View reviewed changes
src/main/java/com/clevercloud/biscuit/token/Biscuit.java
*/
@Deprecated
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was it deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we wanted to expose the ability to pass a root_key_option using vavr. So this method without has been deprecated. But since we want to remove vavr from public methods we keep it and add the one with the root_key_id (not optional) so two methods one with root_key_id and one without.

@divarvel
Copy link
Collaborator

divarvel commented Aug 7, 2023

So the private version takes a java option, and two overloaded variants are exposed? why not.

If you're changing the public interface, something that would be beneficial would be to hide symbol tables.

@KannarFr
Copy link
Contributor Author

KannarFr commented Aug 7, 2023

So the private version takes a java option, and two overloaded variants are exposed? why not.

If you're changing the public interface, something that would be beneficial would be to hide symbol tables.

Good idea. I'll do it in another PR. Let's merge this.

@KannarFr KannarFr merged commit a4790a1 into master Aug 7, 2023
@KannarFr KannarFr deleted the removeVavrOptionalFromPublicMakeMethod branch August 7, 2023 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divarvel divarvel divarvel left review comments

@Geal Geal Awaiting requested review from Geal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove vavr Option from public Biscuit.make method
2 participants
@KannarFr @divarvel