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 alternative ways to write a policy #1513

Merged
merged 10 commits into from
Oct 14, 2024

Conversation

aifrak
Copy link
Contributor

@aifrak aifrak commented Oct 13, 2024

Contributor checklist

  • Bug fixes include regression tests
  • Chores
  • Documentation changes
  • Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

.formatter.exs Outdated Show resolved Hide resolved
end
```

#### Policy with multiple conditions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if it is the right place to mention multiple conditions for one policy.


```elixir
policies do
policy [condition1, condition2, condition3] do
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be sufficient to change the original introduction of checks to contain the list example, which I mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will remove it.


```elixir
policies do
policy always(), authorize_if: always()
Copy link
Contributor

Choose a reason for hiding this comment

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

I worry a bit about adding confusion for these various syntaxes. There isn't really any good reason for someone to have to know that you can use this keyword list syntax. The do block syntax is preferable. I think better to leave this out here, or at the very least put this at the bottom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I did not know there was a preference toward do block. I will remove it.

In my personal project, I was using mostly the keyword list variant, because I find it easier to read. Also it reduce some lines.

```

#### Policy with `condition` inside `do` block

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some explanation here. Like why they might want to specify the condition this way.

Putting the condition inside the policy block can make really long condition lists easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some explanation.

@aifrak
Copy link
Contributor Author

aifrak commented Oct 13, 2024

@zachdaniel Thanks for the feedback. I have made the requested changes. Could you please have a look again?

Copy link
Contributor

@zachdaniel zachdaniel left a comment

Choose a reason for hiding this comment

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

🚀 Thank you for your contribution! 🚀

@zachdaniel zachdaniel merged commit 9280a69 into ash-project:main Oct 14, 2024
8 checks passed
@aifrak aifrak deleted the patch-1 branch October 14, 2024 07:44
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.

2 participants