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

Fix some typos around backticks in docstrings #4463

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Jan 14, 2025

This also removes some faulty spaces between triple backticks and jldoctest and thus enables many doctests in experimental/AlgebraicStatistics (cc @bkholler @taboege @marinagarrote)

@lgoettgens lgoettgens added the documentation Improvements or additions to documentation label Jan 14, 2025
@benlorenz
Copy link
Member

I was looking into adding this to the docstring checks, but then noticed that it should not be necessary, the doctests do run fine with the space after the backticks and even with the extra text after the jldoctest. (the Markdown object is identical so we cannot detect this in the same way as the other docstring error)

For example in the logs on master we have:

page: ~/oscar-runners/runner-01/_work/Oscar.jl/Oscar.jl/experimental/AlgebraicStatistics/src/GaussianGraphicalModels.jl:138-142
  0.000875 seconds (945 allocations: 142.852 KiB)

(for the line with ``` jldoctest directed_ggm)

@lgoettgens
Copy link
Member Author

Good to know @benlorenz.
In this case, I would only see this PR here as a change towards consistency.

The further words like directed_ggm are used to let multiple jldoctests run in the same module/namespace, but this only works on markdown pages or if all with the same further word are part of the same docstring. Thus, here, they do not do anything and could just lead to confusion

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.41%. Comparing base (9627744) to head (599217a).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4463   +/-   ##
=======================================
  Coverage   84.41%   84.41%           
=======================================
  Files         668      668           
  Lines       88598    88598           
=======================================
+ Hits        74786    74788    +2     
+ Misses      13812    13810    -2     
Files with missing lines Coverage Δ
experimental/AlgebraicStatistics/src/CI.jl 90.38% <ø> (ø)
...AlgebraicStatistics/src/GaussianGraphicalModels.jl 96.70% <ø> (ø)
...imental/AlgebraicStatistics/src/GraphicalModels.jl 84.00% <ø> (ø)
experimental/AlgebraicStatistics/src/Markov.jl 82.60% <ø> (ø)
...rimental/FTheoryTools/src/TateModels/attributes.jl 86.58% <ø> (ø)
experimental/ModStd/src/ModStdQt.jl 58.91% <ø> (ø)
experimental/Schemes/src/DerivedPushforward.jl 67.53% <ø> (ø)
...ometry/Surfaces/EllipticSurface/EllipticSurface.jl 83.64% <ø> (ø)

... and 1 file with indirect coverage changes

@taboege
Copy link
Contributor

taboege commented Jan 14, 2025

The further words like directed_ggm are used to let multiple jldoctests run in the same module/namespace, but this only works on markdown pages or if all with the same further word are part of the same docstring. Thus, here, they do not do anything and could just lead to confusion

If I remember correctly, we initially used these "further words" a lot more in AlgebraicStatistics. Our intention was to let doctests run in the same namespace and have a sort of running example through the documentation page that does not repeat the same declarations over and over. I admit that this was misguided to begin with, since examples in the documentation should work out of context. But in any case, we discovered that the doctests with "further words" did not pass (for the reasons you mentioned) and abandoned this idea. It seems like we forgot to completely clean it up.

So this is a welcome change on our part.

@lgoettgens lgoettgens enabled auto-merge (squash) January 14, 2025 11:48
@lgoettgens lgoettgens merged commit 309825e into oscar-system:master Jan 14, 2025
29 of 31 checks passed
@lgoettgens lgoettgens deleted the lg/backtick-typos branch January 14, 2025 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants