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

comments for the require package #1658

Conversation

parth-p-shah
Copy link

Summary

What was asked and done in #1610 , with the review comments addressed.

Changes

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

Motivation

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.

Related issues

#1610 - Generate better comments for require package

@brackendawson
Copy link
Collaborator

Can you base this on @Neokil's work? Ie begin with their branch and add a commit taking the other parts out. You also need to run go generate ./....

@parth-p-shah
Copy link
Author

Can you base this on @Neokil's work? Ie begin with their branch and add a commit taking the other parts out. You also need to run go generate ./....

I'm not able to push into his branch. Also go generate ./... is adding the line:
// Failure of this check is fatal ([testing.T.FailNow] is called on failure).
under every function.

@brackendawson
Copy link
Collaborator

You don't need to push to their branch:

git remote add Neokil [email protected]:Neokil/testify.git
git fetch Neokil
git checkout generate-better-comments-for-require-package
# do work
# commit 
# push to your fork
# open PR

This keeps the correct attribution on the work. You'll need to have go generate working and CI passing before this can be reviewed.

@brackendawson
Copy link
Collaborator

Thank you for your efforts @parth-p-shah, but the author has responded in #1610 now.

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.

2 participants