TidierOrg / Tidier.jl

Meta-package for data analysis in Julia, modeled after the R tidyverse.
MIT License
515 stars 14 forks source link

Add `jldoctest` to docstrings #25

Closed zhezhaozz closed 1 year ago

zhezhaozz commented 1 year ago

This PR adds doctests to docstrings. For example:

"""
    @filter(df, exprs...)

# Examples
```jldoctest 
julia> df = DataFrame(a = repeat('a':'e'), b = 1:5, c = 11:15);

julia> df2 = @chain df begin
       @filter(b >= mean(b))
       end
[ Info: subset(\$(Expr(:escape, :raccoon)), [:b] => ((b,)->coalesce.(b .>= mean(b), false)))
3×3 DataFrame
 Row │ a     b      c     
     │ Char  Int64  Int64 
─────┼────────────────────
   1 │ c         3     13
   2 │ d         4     14
   3 │ e         5     15

When running make.jl, it will run the jldoctest within all docstrings in Julia-REPL and compare the output with the expected output specified.

zhezhaozz commented 1 year ago

@kdpsingh @rdboyes Not sure why the Documenter check gives me this error:

Error: could not evaluate expression from doctest setup.
│   expression = :($(Expr(:toplevel, :(#= /home/runner/work/Tidier.jl/Tidier.jl/docs/make.jl:5 =#), :(using Tidier, DataFrames, Chain, Statistics))))
│   exception =
│    ArgumentError: Package Chain not found in current path, maybe you meant `import/using .Chain`.
│    - Otherwise, run `import Pkg; Pkg.add("Chain")` to install the Chain package.
└ @ Documenter.DocTests ~/.julia/packages/Documenter/H5y27/src/DocTests.jl:188

Feel free to comment or make edits!

kdpsingh commented 1 year ago

The build is failing. It looks like the Info output is causing the problem.

zhezhaozz commented 1 year ago

The build is failing. It looks like the Info output is causing the problem.

Very strange. Didn't have this error when testing locally. Will fix it later

rdboyes commented 1 year ago

Seems like the names that are generated inside of a chain are not consistent? :raccoon vs. :chinchilla - I'm guessing these are autogenerated. Maybe the solution is to have a way to disable the Info: flags? People may not want the DataFrames code showing up every time they run the macros anyway...

kdpsingh commented 1 year ago

You're right. Those are randomly generated, and while they may even be deterministic if run in a fresh session, they are a pain to keep track of and update.

I also agree with you that the info that is outputted isn't really all that helpful, especially as the generated code has gotten more complex. I do want to leave the option of seeing it because it's been incredibly helpful for troubleshooting purposes.

Here's what I'm thinking: @zzhaozheUM, remove all the info-created update and push an updated commit to the PR.

Once you're done, I'll add a function that updates a global variable within the package namespace that determines whether logging will happen. I will set logging to off by default but give users a way to turn it on.

After I fix that, then can have @rdboyes review.

Let me know if that makes sense or if you have any thoughts. Thank you! And have a great weekend.

zhezhaozz commented 1 year ago

@kdpsingh just removed all info-related messages and commented out @info code lines to pass the build.

kdpsingh commented 1 year ago

Excellent! I'll implement a way to turn the code output on through a new exported function but agree with leaving it off as a default.

Once I do that, will ask @rdboyes to take a look before merging.

kdpsingh commented 1 year ago

@rdboyes this is ready for you to take a look.

Going to work on the @group_by and joins today/tomorrow and hoping to release v0.4.0 by the weekend.

kdpsingh commented 1 year ago

Thanks @zzhaozheUM. Can we undo the tabs and leave the old style of formatting where the """ starts on a new line?

zhezhaozz commented 1 year ago

Thanks @zzhaozheUM. Can we undo the tabs and leave the old style of formatting where the """ starts on a new line?

Done

kdpsingh commented 1 year ago

Thank you @zzhaozheUM!

kdpsingh commented 1 year ago

Looks like it is passing all tests. From looking at the build logs (for Documenter and CI, respectively), the doctests and the original tests appear to be running.

I'm going to move forward and merge this commit just so I can move onto the other parts of the code. @rdboyes, if you have any concerns, can file an issue and we will re-evaluate, but I think we are good. Appreciate both your help.

rdboyes commented 1 year ago

Sounds like you two got everything! Sorry, just saw this now, busy day today