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

doctests: switch to the preffered method of calling the tool #8735

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

ulysses4ever
Copy link
Collaborator

@ulysses4ever ulysses4ever commented Feb 4, 2023

which is cabal repl --with-ghc=doctest. This not only catches up with the current upstream recommendations, but also simplifies the usage: before this change calling doctest required you to set up an environment with QuickCheck available (we used to do it via cabal-env). The new method takes care of it automatically.

Also add some docs to fix #8147


Please include the following checklist in your PR:

Please also shortly describe how you tested your change. Bonus points for added tests!

@ulysses4ever
Copy link
Collaborator Author

I really wish fix-whitespace showed me the exact problem, not only the filename. As for the other failure, I think I know how to fix it.

@ulysses4ever ulysses4ever force-pushed the t8504-improve-doctest branch 3 times, most recently from f023545 to bd6e880 Compare February 4, 2023 20:51
@ulysses4ever ulysses4ever marked this pull request as draft February 4, 2023 20:54
@ulysses4ever
Copy link
Collaborator Author

Oh Lord, it's failing even locally now. I can swear to god that yesterday it worked just fine...

@andreasabel
Copy link
Member

I really wish fix-whitespace showed me the exact problem, not only the filename. As for the other failure, I think I know how to fix it.

You can run fix-whitespace locally and it will fix the whitespace problems.
But others have complained before:

@ulysses4ever
Copy link
Collaborator Author

@andreasabel that's what I ended up doing, yes. The issue is: I need to have it locally to do that, and that's not always convenient (I have many environments...). I much rather I saw the issue in the CI and could fix it manually instead of installing it, if it seems easier to me.

@ulysses4ever ulysses4ever marked this pull request as ready for review February 4, 2023 22:07
@ulysses4ever
Copy link
Collaborator Author

All right, fellas. The new method seems to be more sensitive to GHC version. Seems to work fine now.

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Great work!

Maybe the local workflow (Makefile) could be made more robust by having and executing goals for installing doctest for the current GHC version, and checking for correct doctest versions. Some ideas in:

cabal-testsuite/README.md Outdated Show resolved Hide resolved
cabal-testsuite/README.md Show resolved Hide resolved
Makefile Show resolved Hide resolved
.github/workflows/quick-jobs.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@ulysses4ever
Copy link
Collaborator Author

@andreasabel could you please take another look and comment further or approve?

Copy link
Member

@andreasabel andreasabel left a comment

Choose a reason for hiding this comment

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

Thanks for this great work of research and implementation!
(I know what it takes, as I have spent many days just to get doctest working on various projects.)

Makefile Show resolved Hide resolved
cabal-testsuite/README.md Show resolved Hide resolved
@andreasabel andreasabel added re: doctest Concerning doctest suites merge me Tell Mergify Bot to merge and removed attention: needs-review labels Feb 13, 2023
@ulysses4ever
Copy link
Collaborator Author

Thanks @andreasabel! I just stood on the shoulders of giants (nomeata and wismill, in this case). By the way, if you have a spare winter evening, I'm sure you'll enjoy this recent Haskell Discourse thread about various ways of doctesting. (Most of it you probably know by now, though)

@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Feb 15, 2023
which is `cabal repl --with-ghc=doctest`. This not only catches up with
the current upstream recommendations, but also simplifies the usage:
before this change calling doctest required you to set up an environment
with QuickCheck available (we used to do it via cabal-env). The new
method takes care of it automatically.

Also add some docs to fix haskell#8147
@mergify mergify bot merged commit ef1e12c into haskell:master Feb 16, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Feb 23, 2023

@mergify backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 2023

backport 3.10

✅ Backports have been created

mergify bot added a commit that referenced this pull request Feb 24, 2023
doctests: switch to the preffered method of calling the tool (backport #8735)
@ulysses4ever ulysses4ever deleted the t8504-improve-doctest branch April 21, 2023 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: doctest Concerning doctest suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructions on how to run doctests locally
3 participants