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

fix(r/faucet): Render call panic #628

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Conversation

tbruyelle
Copy link
Contributor

@tbruyelle tbruyelle commented Mar 21, 2023

Fix #621

Render prints the controllers in a loop but relies on gControllersSize rather than gControllers.Size() to determine the number of controllers. The fix simply replaces gControllersSize by gControllers.Size().

Fix #627

filetest Output: directive doesn't work when the output contains multiple consecutive empty lines, because the method used to collect those comments truncate them (see issue and ast.CommentGroup.Text()). The fix replaces this method by a custom one that doesn't truncate.


Note if it's preferable, I can split this PR because it fixes 2 different things. I did that because the second fix is required by the first one.

@tbruyelle tbruyelle requested a review from a team as a code owner March 21, 2023 14:02
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Great PR 👍 Some nitpicks and comments on your questions, otherwise looking pretty good.

tests/file.go Outdated Show resolved Hide resolved
examples/gno.land/r/gnoland/faucet/z1_filetest.gno Outdated Show resolved Hide resolved
examples/gno.land/r/gnoland/faucet/z2_filetest.gno Outdated Show resolved Hide resolved
examples/gno.land/r/gnoland/faucet/faucet.gno Show resolved Hide resolved
Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

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

LGTM

@piux2 can you give a last review on this please?

@piux2
Copy link
Contributor

piux2 commented Mar 30, 2023

@tbruyelle solid!

Thank you for adding testing cases for rendering results.
fixing ast.CommentGroup.Text() is awesome.

tbruyelle and others added 5 commits April 3, 2023 18:03
Fix gnolang#621

`Render` prints the controllers in a loop but relies on
`gControllersSize` rather than `gControllers.Size()` to determine the
number of controllers. The fix simply replaces `gControllersSize` by
`gControllers.Size()`.

Fix gnolang#627

filetest `Output:` directive doesn't work when the output contains
multiple consecutive empty lines, because the method used to collect
those comments truncate them (see issue and `ast.CommentGroup.Text()`).
The fix replaces this method by a custom one that doesn't truncate.
Co-authored-by: Morgan <git@howl.moe>
@tbruyelle
Copy link
Contributor Author

Rebased on master, ready to merge.

@moul moul merged commit f8231e0 into gnolang:master Apr 3, 2023
40 checks passed
grepsuzette pushed a commit to grepsuzette/gno that referenced this pull request Apr 6, 2023
Co-authored-by: Morgan <git@howl.moe>
Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

ast.CommentGroup.Text() shouldn't be used to read expected test's outputs r/gnoland/faucet.Render panics
4 participants