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

chore: remove No*Permissions structs #12316

Merged

Conversation

lucacasonato
Copy link
Member

These are confusing. They say they are "for users that don't care about
permissions", but that isn't correct. NoTimersPermissions disables
permissions instead of enabling them.

I would argue that implementors should decide what permissions they want
themselves, and not take our opinionated permissions struct.

These are confusing. They say they are "for users that don't care about
permissions", but that isn't correct. `NoTimersPermissions` disables
permissions instead of enabling them.

I would argue that implementors should decide what permissions they want
themselves, and not take our opinionated permissions struct.
@bartlomieju
Copy link
Member

I would argue that implementors should decide what permissions they want
themselves, and not take our opinionated permissions struct.

This was originally added so that implementors can use default structs if they don't care about permissions 🤷 I don't have a strong opinion either way. I'll defer to @ry for review, whose idea it was

@lucacasonato lucacasonato requested a review from ry October 4, 2021 19:21
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - This will induce some changes in Deploy

@lucacasonato lucacasonato merged commit 64a7187 into denoland:main Oct 4, 2021
@lucacasonato lucacasonato deleted the remove_no_-_permissions_structs branch October 4, 2021 21:29
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