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

Begin moving examples to Literate.jl and incorporate into documentation #2375

Merged
merged 13 commits into from
Nov 23, 2020

Conversation

odow
Copy link
Member

@odow odow commented Nov 19, 2020

TODOs

  • Print statements don't appear in the output unless they are the final statement in a cell
    image

image

This worked quite well. We have two options:

  1. re-write examples, splitting into annotated parts

    Pros:

    • reads better

    Cons:

    • Hiding the tests means we need to run file twice, once with doc-test excluding @test and once as part of tests.
    • More work to re-write
    • No easy copy-paste of the full example
  2. minimal changes. Keep tests and function structure.

    Pros:

    • less work to re-write
    • Can run doctests with @test.
    • Easy copy-paste of the full example

    Cons:

    • Less readable?

I think I prefer option 2.

Option 1

image

Option 2

image

@codecov
Copy link

codecov bot commented Nov 19, 2020

Codecov Report

Merging #2375 (5cfd886) into master (a2341d8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2375   +/-   ##
=======================================
  Coverage   91.54%   91.54%           
=======================================
  Files          42       42           
  Lines        4448     4448           
=======================================
  Hits         4072     4072           
  Misses        376      376           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2341d8...5cfd886. Read the comment docs.

@joaquimg
Copy link
Member

Can't we use #hide instead of #src so it runs once?

We could link to the .jl file in the git repo so that it's trivial to copy the entire thing and past.

Plus there is the binder thing that is used in https://jump.dev/SumOfSquares.jl/stable/generated/sos_decomposition/

@mlubin
Copy link
Member

mlubin commented Nov 20, 2020

This looks great! If we choose option 2, it will look a bit weird to have print statements that don't display, right?

@odow
Copy link
Member Author

odow commented Nov 20, 2020

it will look a bit weird to have print statements that don't display, right?

Yes. But I assume I can fix this somehow. My preference is for option 2 with prints.

@odow
Copy link
Member Author

odow commented Nov 20, 2020

There are also some style questions. I wonder about only having using JuMP and all other packages be import XXX so it's clear which functions are not part of JuMP.

@blegat
Copy link
Member

blegat commented Nov 20, 2020

I'm in favor of option 1. There is indeed more work for CI having to run it 3 times: for the doc, for the notebook and for the tests. However, the improvement of user experience seems to be worth it.

We could link to the .jl file in the git repo so that it's trivial to copy the entire thing and paste.

I agree

@odow
Copy link
Member Author

odow commented Nov 22, 2020

  • I found the issue with the printing. It's only if the return statement is a test.

  • I've also moved all the examples to /docs/src/examples. I think this is a win because it removes the /examples folder that people stumble across when they hit the GitHub page. The documentation should point to each raw .jl file when you click the "Edit on Github" button at the top. I don't think this works locally because it doesn't support the relative links -- it's something to check once we merge. If it doesn't work, we can add extra links in a separate PR.

  • The diff is huge because there was a massive MPS file in /examples/data?

  • I removed the extra examples CI run. The documentation run now evaluates the raw .jl files first, then strips out any lines ending in #src, converts them to markdown.

  • Some of the examples I've converted to option 1, and most I've left as option 2. I think merging as-is is a big win.

  • I re-arranged the navigation bar, moving the current docs into "/Manual" and collapsing the first nested level:

image

Copy link
Member

@blegat blegat left a comment

Choose a reason for hiding this comment

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

Let's merge!

@odow odow merged commit 9b6cdac into master Nov 23, 2020
@odow odow deleted the od/examples branch November 23, 2020 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants