Skip to content

Conversation

@megoth
Copy link
Contributor

@megoth megoth commented Aug 5, 2019

No description provided.

@megoth megoth requested a review from timbl August 5, 2019 18:59
Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

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

Being finicky, as we have that test ow twice on each recursive call to tryParent, maybe it should e replaced by a single test at the beginning of tryParent, before the getACL .

In fact the test left >= right will catch https:// (left and right will both be 8) but won't catch https://foo.com/ where left by 8 and right 16. So the right test was in the wrong place?

@megoth
Copy link
Contributor Author

megoth commented Aug 6, 2019

Being finicky, as we have that test ow twice on each recursive call to tryParent, maybe it should e replaced by a single test at the beginning of tryParent, before the getACL .

Right, I thought it would work since it was the logic being done already at https://github.com/solid/solid-ui/blob/f09f540bbcedbeb8085d7c25b65908ae48909820/src/acl.js#L348 . But yes, We should do that test before making the call.

In fact the test left >= right will catch https:// (left and right will both be 8) but won't catch https://foo.com/ where left by 8 and right 16. So the right test was in the wrong place?

But https://foo.com/.acl might also be a ACL resource we want to check? I.e. when a user has a single user Pod server setup that serves their domain? Maybe I misunderstand you, but I think the test left >= right is the only one we need to check for?

@megoth
Copy link
Contributor Author

megoth commented Aug 6, 2019

@timbl I've changed the code based on your feedback now.

I also found a place where you had forgotten to add the ACL('default')-fix.

Lastly, I added a TODO for you to review - didn't really find much use of that method in the projects I have in my current setup (NSS, mashlib, solid-panes, solid-ui, rdflib), so uncertain of it's usage. Might be a method we want to mark as obsolete and remove at next major release?

Copy link
Contributor

@timbl timbl left a comment

Choose a reason for hiding this comment

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

Beware of "^0.21.1" for rdflib maybe ">=0.21.1" ... maybe personal preference

@megoth
Copy link
Contributor Author

megoth commented Aug 7, 2019

Beware of "^0.21.1" for rdflib maybe ">=0.21.1" ... maybe personal preference

I think this is how NPM does it by default, so didn't think more of it.

@megoth megoth merged commit 9e9a69f into dev Aug 7, 2019
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