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

Generate better comments for require package #1610

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Neokil
Copy link

@Neokil Neokil commented Jun 13, 2024

Summary

The comments for the require package were just copied over from the assert package when generating the functions. This could lead to confusion because

  1. The code-examples were showing examples using the assert package instead of the require package
  2. The function-documentation was not mentioning that the functions were calling t.FailNow() which is some critical information when using this package.

Changes

  1. Added a "replace" function to the template
  2. require.go.tmpl now replaces the package name accordingly
  3. require.go.tmpl now adds another line to the function-docs explaining the difference to the assert-function

Motivation

When new developers are coming in they most likely did not go through the github-documentation for every package used in the application. But what they do is read the function-docs when they go through code to understand what it is doing.
In case of test-cases using both require and assert it gets confusing because the function docs for the require package are referencing the assert package and don't really document the actual function.

The comments for the require package were just copied over
from the assert package when generating the functions.
This could lead to confusion because
1. The code-examples were showing examples using the
assert package instead of the require package
2. The function-documentation was not mentioning that
the functions were calling `t.FailNow()` which is some
critical information when using this package.
require/require.go.tmpl Outdated Show resolved Hide resolved
@dolmen dolmen added pkg-require Change related to package testify/require internal/codegen Change related to internal code generation documentation labels Jun 13, 2024
@dolmen
Copy link
Collaborator

dolmen commented Jun 13, 2024

I've just merged #1607, so please rebase and run codegen.

@hendrywiranto
Copy link
Contributor

this will close #1609 too

@Neokil Neokil requested a review from dolmen June 17, 2024 07:16
@Neokil
Copy link
Author

Neokil commented Jun 25, 2024

somehow I cannot add reviewers the to the PR, but maybe @boyan-soubachov, @dolmen, @MovieStoreGuy, @arjunmahishi or @brackendawson can one of you have a look?

@Neokil
Copy link
Author

Neokil commented Jul 31, 2024

any change to get someone to review/merge this?

Copy link
Collaborator

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

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

The change to swap assert. with require. is good. 👍 It should be safe as if any assert examples ever use msgAndArgs because the syntax will be compatible with require. I considered if inline examples should use the package name at all, in the standard library they do, despite links to functions not doing.

Adding a note about t.FailNow to every requirement is too much. The doc for require already says they stop test execution, we should just change this sentence to mention that it uses t.FailNow. The README.md also mentions t.FailNow, but these two docs should probably match one-another.

@brackendawson
Copy link
Collaborator

brackendawson commented Oct 9, 2024

this will close #1609 too

I'm afraid it wont, requre.Error has no return value. That docstring needs to be made into something that will work for both packages.

@Neokil
Copy link
Author

Neokil commented Oct 15, 2024

@brackendawson just removed the additional comment as you requested.

I have to say that IMO a hint like this would be really helpful after all to all people that are new to the package. When you encounter it in an existing project you will only see the function-comments in your IDE with no hint to the t.FailNow(). It is only when you are actually looking into the Github of the library that you will find that information. If you land on the pkg.go.dev page because you want to see the documentation for the function to see why it terminated your test you need to know that you have to scroll up to the top or else you will not find that info.

@brackendawson
Copy link
Collaborator

I have to say that IMO a hint like this would be really helpful after all to all people that are new to the package. When you encounter it in an existing project you will only see the function-comments in your IDE with no hint to the t.FailNow(). It is only when you are actually looking into the Github of the library that you will find that information. If you land on the pkg.go.dev page because you want to see the documentation for the function to see why it terminated your test you need to know that you have to scroll up to the top or else you will not find that info.

I hear you. And it's not that I disagree with any of that. But there are a lot of other notes that we could add to each function's docstring and there is a point where we're better not making them larger. For me the concept of t.Error vs t.Fatal doesn't feel so surprising, even if you might not be familiar with our nomenclature of assert and require. Just my opinion.

Going to allow some time for other maintainers to review before I merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation internal/codegen Change related to internal code generation pkg-require Change related to package testify/require
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants