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

ast: Return an error when parsing an empty module #2060

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

patrick-east
Copy link
Contributor

The documentation is pretty clear that a module must at least contain
a package, so it is safe to say that an empty file isn't valid.

Previously the helper would just return a nil module and nil error, it
will now return an error.

Fixes: #2054
Signed-off-by: Patrick East east.patrick@gmail.com

@patrick-east patrick-east requested a review from tsandall February 5, 2020 22:54
tsandall
tsandall previously approved these changes Feb 17, 2020
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.

Maybe you've already done this but it would be good to review the callers of ParseModule to remove any unnecessary checks for the nil, nil case now. I'm guessing most weren't doing it properly since this is not an obvious return value.

Other than that, since this change is backwards incompatible, let's put a small release note in place.

@patrick-east
Copy link
Contributor Author

it would be good to review the callers of ParseModule to remove any unnecessary checks for the nil, nil case now

Done, and removed a handful of spots.

I'm guessing most weren't doing it properly since this is not an obvious return value.

Yea.. it was a very small minority that would check for it.

@patrick-east patrick-east force-pushed the issue/2054 branch 2 times, most recently from ab964b2 to 1ee5b20 Compare February 19, 2020 22:17
@tsandall
Copy link
Member

Let's merge this after we cut the v0.17.2 release to minimize the impact.

@tsandall
Copy link
Member

@patrick-east let's merge this now that v0.17.2 is out. Just need to resolve the changelog conflict..

The documentation is pretty clear that a module must at least contain
a package, so it is safe to say that an empty file isn't valid.

Previously the helper would just return a nil module and nil error, it
will now return an error.

Fixes: open-policy-agent#2054
Signed-off-by: Patrick East <east.patrick@gmail.com>
@patrick-east patrick-east merged commit 55b474a into open-policy-agent:master Feb 21, 2020
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.

PrepareForEval panics on empty input
2 participants