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

format: don't group iterable when one has an empty file name #4260

Merged
merged 3 commits into from
Jan 25, 2022

Conversation

srenatus
Copy link
Contributor

@srenatus srenatus commented Jan 21, 2022

As mentioned in the comment, empty file names happen when the format
package's Ast() function does a sweep of its input, and adds a
"default location" to everything that has a nil location.

During PE, when generated the pairs to save in saveUnify, we'll
return Var Terms without locations. Fixing that seemed like a bigger
hurdle, so I went this route.

The new check is such that if any term has the default file in
its location, such as would happen if we're formatting code that
was created programmatically (not parsed), we'll group the terms'
elements, but print them in one line.

@srenatus srenatus marked this pull request as draft January 21, 2022 11:15
@srenatus srenatus marked this pull request as ready for review January 21, 2022 11:35
@srenatus
Copy link
Contributor Author

There is one problem still that I don't see how to fix without
extensive changes to the formatter architecture:
If you partially evaluate an ast.Module without file names in
its terms' locations, then there will not a way to discern,
in formatting, if a wildcard has a genuine location or one
made up by the formatter. The workaround is to give the ast.Module
a filename, even if it's just a made-up one.

☝️ We can discern it if we give the defaultLocation ast.Location's a filename that is not "" but "__default__". I think it would be safer to go that route.

@srenatus srenatus marked this pull request as draft January 21, 2022 21:19
As mentioned in the comment, empty file names happen when the format
package's Ast() function does a sweep of its input, and adds a
"default location" to everything that has a nil location.

During PE, when generated the pairs to save in saveUnify, we'll
return Var Terms without locations. Fixing that seemed like a bigger
hurdle, so I went this route.

The new check is such that if any term has the default file in
its location, such as would happen if we're formatting code that
was created programmatically (not parsed), we'll group the terms'
elements, but print them in one line.

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>
@srenatus
Copy link
Contributor Author

Updated. RFR.

@srenatus srenatus marked this pull request as ready for review January 23, 2022 12:16
@@ -336,6 +336,52 @@ a[_x[y]]`,
expected: `_x
a[_x[y][[z, w]]]`,
},
{
Copy link
Member

Choose a reason for hiding this comment

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

Do we have an example of how this would look prior to this fix ? I would help to understand this fix (at least to me 😅 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[
    _,
    "foo",
] = split( ... )

🙈

Copy link
Member

@tsandall tsandall left a comment

Choose a reason for hiding this comment

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

This fix seems reasonable to me... if the iterable contains elements that do not have locations, the iterable cannot be sorted into groups, so just leave the iterable as-is. LGTM

@srenatus srenatus merged commit 932e4ff into open-policy-agent:main Jan 25, 2022
@srenatus srenatus deleted the sr/format/group-wildcards branch January 25, 2022 07:43
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.

None yet

3 participants