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

docs: add guide for rewrites #948

Closed
wants to merge 5 commits into from
Closed

Conversation

hperl
Copy link
Collaborator

@hperl hperl commented Aug 11, 2022

This PR adds documentation for the userset rewrites.

It is blocked by ory/keto#877, but the content can already be reviewed.

@hperl
Copy link
Collaborator Author

hperl commented Aug 11, 2022

/cc @zepatrik @tomekpapiernik

Copy link
Contributor

@tomekpapiernik tomekpapiernik left a comment

Choose a reason for hiding this comment

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

These are some nicely written documents :) Please have a look at the specific comments I left and address them.

One general remark is that it would be awesome if you could get rid of the "we" form when describing subsequent steps of some process. You're creating instructions that the user has to follow to get certain results, so it's totally okay to use imperative forms, for example:

We use the following relation tuples to showcase the namespace configuration.
turn it into
These relation tuples showcase namespace configuration.

There's no need for this undefined "we" in the docs ;) I hope you know what I mean. Let me know if you need any help! I'll be happy to take another look, please let me know when you're ready.

@hperl hperl force-pushed the keto-userset-rewrites branch from 12cf214 to dd5cbcc Compare August 25, 2022 14:06
@hperl hperl requested a review from tomekpapiernik August 25, 2022 14:06
@hperl hperl force-pushed the keto-userset-rewrites branch from dd5cbcc to 20ec739 Compare August 25, 2022 14:13
@hperl hperl self-assigned this Sep 1, 2022
Co-authored-by: Tom Papiernik <[email protected]>
@hperl hperl force-pushed the keto-userset-rewrites branch from 72b7cae to 321829b Compare September 1, 2022 15:50
@hperl hperl requested a review from tomekpapiernik September 1, 2022 15:51
@hperl
Copy link
Collaborator Author

hperl commented Sep 1, 2022

Hi @tomekpapiernik, I addressed all comments :).

Copy link
Contributor

@tomekpapiernik tomekpapiernik left a comment

Choose a reason for hiding this comment

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

Looking better now, still a few more things left to fix though ;)

@aeneasr
Copy link
Member

aeneasr commented Sep 12, 2022

The feature has long been shipped and the PR is open for a month, I will get this over the finish line. Please make the minor language changes which are not blocking in a follow up PR.

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