Skip to content

Ensure that the testsuite doesn't generate unchecked temporary files#7483

Merged
dlang-bot merged 2 commits intodlang:masterfrom
wilzbach:check-clean-git
Dec 24, 2017
Merged

Ensure that the testsuite doesn't generate unchecked temporary files#7483
dlang-bot merged 2 commits intodlang:masterfrom
wilzbach:check-clean-git

Conversation

@wilzbach
Copy link
Copy Markdown
Contributor

So that #7482 isn't needed anymore in the future.

@dlang-bot
Copy link
Copy Markdown
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

posix.mak Outdated

check-clean-git:
@git diff-index --quiet HEAD -- || \
(echo "ERROR: You created temporary files. Store them in tests_results or remove them."; git status; exit 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The error message sounds a bit too much like it is accusing the current user, which may not be at fault for the temporary files. How about "ERROR: Found residual temporary files. Temporary files should be stored in test_results or explicitly removed."

Also, the git status tends to overwhelm the error message, making it less obvious. Perhaps use git status -s which displays a less verbose output. In that case you may want to change the error message to "ERROR: Found the following residual temporary files. Temporary files should be stored in test_results or explicitly removed."

Example output:

ERROR:  Found the following residual temporary files.  Temporary files should be stored in `test_results` or explicitly removed.
?? dub.selections.json

Sorry for being so picky about such things. I can't help it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, it might be helpful to add a comment with a link to this PR. It helps when maintaining these makefiles to understand why certain things exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for being so picky about such things. I can't help it.

No that's cool. It really helps to make a better user experience!

mkdir -p $(INSTALL_DIR)/man
cp -r docs/man/* $(INSTALL_DIR)/man/

check-clean-git:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Major targets should have documentation explaining what they do. Otherwise, the user is left to puzzle out the commands executed, which (like all code) isn't always obvious.

Copy link
Copy Markdown
Member

@WalterBright WalterBright left a comment

Choose a reason for hiding this comment

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

document check-clean-git

@wilzbach wilzbach force-pushed the check-clean-git branch 2 times, most recently from 6863645 to 18c1484 Compare December 22, 2017 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants