Fix --output-document flag

[amaury1093]

Description

ref: #8109

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[alessio]

@amaurymartiny -> #8122 - please take a look

[alexanderbez]

This function is wayyyy to large for naked returns. If using named return value(s), please avoid naked returns and explicitly return the value(s).

[alessio]

I've made a couple of corrections @alexanderbez. The naked returns allow the function to return the error returned by a -*File.Close() call in case no write errors were reported before. There is only one return value. So thought that the impact on readability wouldn't be too big. Wdyt?

(There is an excellent article that illustrates the pattern that I used and its rationale: https://www.joeshaw.org/dont-defer-close-on-writable-files/#:~:text=But%20this%20idiom%20is%20actually,infrequent%2C%20maddening%20bugs%20will%20occur.)

[alexanderbez]

I do not understand. It's as simple as return err. Please do not use naked returns. I don't want to read the code and guess what err is.

[alessio]

@amaurymartiny I had a chat with bez. My code suggestions should both just work and make him happy

[amaury1093]

Weird error Unregistered interface types.PubKey, adding a codec.RegisterCrypto(clientCtx.LegacyAmino) solves it, but I'll investigate a bit why it's not registered upstream already.

[alessio]

This may cause the test to fail if the temporary file is not deleted successfully at the end of the test (which might happen for a number of reasons that are entirely unrelated to the test case itself)

[amaury1093]

ah, what are best practices here? should I just ignore the err?

[alexanderbez]

I do not know if I agree with this. I would keep this as-is. If it errors, so be it. Just rerun the test. Ignoring errors in tests is typically not something you want to do.

[alessio]

This is about the removal of a temporary file that is created within the execution of a test. If we want to go for The Right Way(c), then yes we might want to cause the test fail. Though weird things happen in a CI that we don't fully control, e.g. resources might become unavailable before the program exits. In my opinion, it'd just be a self-inflicted pain in the lower back if we had to go and manually re-run all tests because of a glitch in the CI. Thus we should just ignore the error and let go.

[amaury1093]

Solved with #8121 (comment), I guess.

[amaury1093]

OK i realized that @robert-zaremba already fixed this in #8106. Talked offline with Robert, i'll close this and let's continue there.