-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add a suggestion to match the package name and file location #35
Conversation
Signed-off-by: Charlie Egan <[email protected]>
Signed-off-by: Charlie Egan <[email protected]>
2762a21
to
68847ec
Compare
68847ec
to
fbf4539
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.
Thanks for doing this! We've had some discussons on this yesterday, so sorry from dropping that here now 😅 But good to get to the bottom of the details before we publish.
style-guide.md
Outdated
@@ -51,6 +51,8 @@ Rego policies. If you enjoy this style guide, make sure to check it out! | |||
* [Avoid using the last argument for the return value](#avoid-using-the-last-argument-for-the-return-value) | |||
* [Regex](#regex) | |||
* [Use raw strings for regex patterns](#use-raw-strings-for-regex-patterns) | |||
* [Packages](#packages) | |||
* [Package name should make file location](#package-name-should-make-file-location) |
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.
make -> match?
style-guide.md
Outdated
@@ -200,11 +206,13 @@ developer experience as well as the quality of your policies. | |||
The built-in functions use `snake_case` for naming — follow that convention for your own rules, functions, and variables. | |||
|
|||
**Avoid** | |||
|
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.
I disabled this rule in the Regal repo, as IMHO, it just makes the code blocks feel detached... but I don't feel strongly about it. Fine to change here.
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.
Reverted in a620cb3
|
||
```rego | ||
# foo/bar.rego | ||
package bar.foo |
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.
There's been some discussion between me and @srenatus about this yesterday, and there's basically two ideas here (not necessarily represented by any of us 😆):
-
Only directory structure (and not filename) should mirror the package path. Otherwise, you'd be forced to have all your code for any given package kept in a single file, which isn't ideal. See for example the number of files contributing to
regal.ast
. I kinda like this idea, but one unresolved issue with it is how to deal with tests. Surely we don't want to recommend using afoo/bar_test
directory forfoo.bar_test
? So if we go with this option, I think tests should only need to match up their package path with the directory structure up until_test
. -
Directory structure and filename should contribute to the package path. This is basically how the
bundle/regal/rules
directory works looks right now, and since there's always only one file per rule (+ test), this has worked well. But while we don't have the problem of multiple files contributing to a single package, it's hard to make that a general recommendation.
Since this is the Rego Style Guide and not Regal, perhaps we can simply describe both of these approaches with their pros and cons, and then settle for one default in Regal, but allow configuration toggles for flexibility.
I think that what matters most is not how exactly, but that this is done consistently across a project.
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.
I've added an updated version in 9b7f0f5
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.
LGTM!
4c7c854 is the new content.