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

show that facts can be retrieved from authorizer #70

Conversation

time4tea
Copy link
Contributor

@time4tea time4tea commented Aug 6, 2023

reference #69

give terms 'getters'
don't expose HashSet impl in 'getter'
replace hashcode with java versions.
make values final

hope its useful

I agree to https://developercertificate.org/

give terms 'getters'
don't expose HashSet impl in 'getter'
replace hashcode with java versions.
make values final
@time4tea
Copy link
Contributor Author

time4tea commented Aug 7, 2023

hey thanks for all the comments - pr was to be useful. if you would like it go ahead, but its not really up for comments. cheers!

@divarvel
Copy link
Collaborator

divarvel commented Aug 7, 2023

Thanks for the PR! I have added comments for things I know (text representation of datalog terms). I realize that you kept existing behaviour, but since you're changing this part, we might as well make it consistent with other implementations.

For the hashcode modifications, someone who actually knows java will be better suited than me.

@time4tea
Copy link
Contributor Author

time4tea commented Aug 7, 2023

i dont intend to make any further modifications - please take or not as you wish. cheers!

@divarvel
Copy link
Collaborator

divarvel commented Aug 7, 2023

alright, i'll let @KannarFr decide then. I still think the changes i mentioned are important but they can be carried out in a follow-up PR indeed.

Thanks for your time

@KannarFr
Copy link
Contributor

KannarFr commented Aug 7, 2023

LGTM.

Can you write an issue about the suggestions you've made?

@time4tea
Copy link
Contributor Author

time4tea commented Aug 7, 2023

not trying to be unhelpful - but also want scope to be limited. change of other behaviours can be addressed later - and maybe i can even do them - but don't want to expand to include other items...
(arbitrary) scope was: add test to show current behaviour of system, including missing feature, add feature, address IDE warnings.

@KannarFr
Copy link
Contributor

KannarFr commented Aug 7, 2023

I agree with that. I want to trace the related issue and quote it there before merging this. To prevent us from forgetting it.

@time4tea
Copy link
Contributor Author

time4tea commented Aug 7, 2023

oh - the issue was #69 (also in the pr desc) - unless i misunderstood?

@KannarFr KannarFr merged commit 5d0016d into biscuit-auth:master Aug 7, 2023
1 check passed
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.

3 participants