-
-
Notifications
You must be signed in to change notification settings - Fork 347
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
feat: add spec for namespace configs #883
Conversation
bf3b305
to
411a86a
Compare
@zepatrik @aeneasr I'm now done with the spec. We landed on a design that is a strict subset of TypeScript, which will enable us to use a whole lot of tooling, starting with type information to guide the user, linting (with eslint) to detect common problems etc. The configuration language is still easy enough to map directly to the Zanzibar AST, as I have documented in the spec. Once the spec is merged, I will add a diagram through mermaid.js. I already started, but keeping the diagram up to date with the spec changes was tedious during the draft. /cc @vinckr |
/cc @kevgo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for writing this up! This is really cool, I'm very excited to start playing with this idea more! I consider this idea of using TS syntax pre-alpha stage, so here comes relatively fundamental feedback. Curious what you think!
bfd4a57
to
d37c63a
Compare
@kevgo thank you for your comments. I addressed your suggestions, and especially liked the improved wordings for Some of the things you mentioned (adding keywords, builtins), can be moved to a next version of the spec, as they will be backwards-compatible. The only thing that is still open is the discussion about |
7c76024
to
9ce5f05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome collaboration on this, thanks for all the thoughful comments! I totally agree with keeping this as simple as possible. Another round of feedback after sleeping over it again. This is coming along nicely!
Agreed! I just incorporated the latest comments and am quite happy with the result. I noticed that we can sidestep the equality discussion because we are only testing for set membership for now anyways (and there are good arguments for it staying that way). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next round of feedback. I limit the amount of ideas in each review to keep them manageable. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO
- remove metadata
- parens in boolean expr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
address review comments
6c4ff84
to
572c0b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, that's all the feedback I have from my end. Next steps after the last changes of the spec are done:
- polish of the language using grammarly.com (I can do this, want to wait after all other edits are done since this will cause lots of conflicts with concurrent edits)
- format this file using Prettier
0523960
to
1881793
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the suggestions from grammarly.com. Mostly removing passive voice. Please run Prettier again after you add these changes.
Thanks, @kevgo! I (think) I applied all of your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has turned out really nice! Can't wait to build some permission sets in it!
Co-authored-by: Kevin Goslar <kevin.goslar@gmail.com> Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
a83b720
to
1586af5
Compare
This PR adds the first version of the Ory Permission language spec to the repo.
Two (last) open issues: