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

refactor: adjust DSL indentation in tests inside package server #1597

Closed

Conversation

00chorch
Copy link
Contributor

@00chorch 00chorch commented May 4, 2024

Fixes #1302 for package server

Description

Now that openfga/language parser has better support for whitespaces, refactor to
have better readability in the test files.

References

Issue #1302

Review Checklist

  • I have clicked on "allow edits by maintainers".
  • I have added documentation for new/changed functionality in this PR or in a PR to openfga.dev [Provide a link to any relevant PRs in the references section above]
  • The correct base branch is being used, if not main
  • I have added tests to validate that the change in functionality is working as expected

@00chorch 00chorch requested a review from a team as a code owner May 4, 2024 03:33
@@ -46,14 +46,14 @@ func TestListObjectsDispatchCount(t *testing.T) {
{
name: "test_direct_relation",
model: `model
schema 1.1
schema 1.1
Copy link
Member

Choose a reason for hiding this comment

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

This is really nested - can you put the model clause on a newline below and start everything inline from there. For example,

model: `
model
  schema 1.1

type user
type document
  relations
    define viewer: [user]
 `,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback @jon-whit
I wrote a script for this. I'll adjust it to move the content after ` to be in a new line aligned at column 0

Here's a new example with the expected output

	model := testutils.MustTransformDSLToProtoWithID(`
model
schema 1.1

type user

type document
  relations
    define viewer: [user, document#viewer] or editor
    define editor: [user, document#viewer]`)

Once approved, I can take care of other packages by just re-running the script

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.34%. Comparing base (aa623fb) to head (3b76ad4).

❗ Current head 3b76ad4 differs from pull request most recent head 3fdf297. Consider uploading reports for the commit 3fdf297 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1597      +/-   ##
==========================================
- Coverage   86.39%   86.34%   -0.04%     
==========================================
  Files          86       86              
  Lines        8224     8224              
==========================================
- Hits         7104     7100       -4     
- Misses        789      792       +3     
- Partials      331      332       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@00chorch 00chorch requested a review from jon-whit May 8, 2024 02:40
Comment on lines +48 to +57
model: `
model
schema 1.1

type user
type user

type folder
relations
define viewer: [user]
`,
type folder
relations
define viewer: [user]
`,
Copy link
Member

Choose a reason for hiding this comment

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

I liked it better the way it was before
image

Comment on lines +241 to 250
testutils.MustTransformDSLToProtoWithID(`
model
schema 1.1
type user

type document
relations
define viewer: [user]`), nil)
relations
define viewer: [user]`), nil)

mockDatastore.EXPECT().
Copy link
Member

Choose a reason for hiding this comment

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

I like the model definition to be aligned vertically with whatever comes before it

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: accidentally closed PR. I will resubmit respecting previous lines indentation. ETA today/tomorrow

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.

Update openfga/language Go module for DSL transformer/parser and refactor usages
3 participants