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

DOC backreferences_dir is now mandatory #307

Merged
merged 5 commits into from Feb 20, 2018

Conversation

Projects
None yet
5 participants
@NelleV
Contributor

NelleV commented Oct 20, 2017

so adding it to the fast set up page.

@NelleV NelleV force-pushed the NelleV:doc_ branch 2 times, most recently from aa0bb80 to b7b2dfc Oct 20, 2017

@Titan-C

This comment has been minimized.

Member

Titan-C commented Oct 21, 2017

@NelleV

This comment has been minimized.

Contributor

NelleV commented Oct 21, 2017

That change doesn't get backreferences to work: it just avoids sphinx-gallery's red warning messages, which I really don't expect when setting up a project. By default, it should work without warnings.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Oct 21, 2017

I agree with @NelleV that "it should work out of the box without warnings" is a good first-experience with sphinx-gallery. Warnings freak a lotta people out, even if we are used to simply ignoring them half the time :-)

@lesteve

This comment has been minimized.

Contributor

lesteve commented Oct 27, 2017

IIRC the warning was to gently nudge people towards updating their config (old way of doing things was mod_examples_dir). It was done in dd661dd which is in 0.1.9 *6 months ago April 20th 2017). We have released 0.1.13 recently.

Maybe this is enough time to remove the warning. Alternatively we could only warn if we detect the old mod_example_dir key in the sphinx config.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Oct 27, 2017

@NelleV

This comment has been minimized.

Contributor

NelleV commented Oct 27, 2017

I agree as well.

@Titan-C

This comment has been minimized.

Member

Titan-C commented Oct 31, 2017

Ok. Let's change the warning setup. Put the warning when the old key is found.

Nevertheless I'm still unsure about the default. What is getting backreferences to work? In my most common use case it is to configure autosummary to use them to extend the API documentation. In which case just declaring the directory where they are stored is not enough, there are some extra steps you need to be careful about. I rather make the default False and direct the user to the docs on how to setup this feature correctly.

  • If to work is to just store them in a directory, yes we could suggest a default directory in the quickstart guide in the main readme.

@Titan-C Titan-C referenced this pull request Feb 11, 2018

Closed

v0.1.14 release plan #344

@choldgraf choldgraf added this to the v0.1.14 milestone Feb 11, 2018

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 11, 2018

hey all - looking through the comments, it looks like this is what needs to be done here:

  • Raise a warning if mod_example_dir is in there
  • Set the default value to None (or False, whichever works better) and include in the comment a link to the docs on how to set this up.

That it? @NelleV is this is something you have time to clean up, or do you want someone else to take over the PR?

@NelleV

This comment has been minimized.

Contributor

NelleV commented Feb 11, 2018

Someone can take over the PR (unless it waits until we resubmit the paper)

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 12, 2018

@NelleV I don't think this should be a tough fix*, I can just push to this PR and we can move from there

*famous last words

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 14, 2018

Just pushed changes to the PR, it now reverts the documentation addition, but now makes the warning only come up if it detects mod_example_dir!

@codecov-io

This comment has been minimized.

codecov-io commented Feb 14, 2018

Codecov Report

Merging #307 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
- Coverage   91.98%   91.93%   -0.06%     
==========================================
  Files          27       27              
  Lines        1847     1834      -13     
==========================================
- Hits         1699     1686      -13     
  Misses        148      148
Impacted Files Coverage Δ
sphinx_gallery/gen_gallery.py 86.04% <100%> (-0.48%) ⬇️
sphinx_gallery/tests/test_gen_gallery.py 100% <100%> (ø) ⬆️

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 4993065...59fa557. Read the comment docs.

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 14, 2018

ok I think this is ready for you to take a look @Titan-C :-)

NelleV and others added some commits Oct 20, 2017

Some tweaks:
* do not warn when backreferences_dir is None (default)
* test that no warning is issued for a simple config
* minor tweaks to warning message when mod_example_dir is used

@lesteve lesteve force-pushed the NelleV:doc_ branch from 84daf36 to 59fa557 Feb 19, 2018

@lesteve

This comment has been minimized.

Contributor

lesteve commented Feb 19, 2018

I pushed a few tweaks:

  • do not warn and set a default backreferences_dir when backreferences_dir is None. I think this was supposed to be nice for users that were using the default magical mod_example_dirs. I am pretty sure that this was not working for most projects (unless you had exactly the same set-up as scikit-learn). Also as I mentioned above this warning was here for 4 sphinx-gallery releases so this is another argument to remove the warning.
  • test that no warning is issued for a simple config
  • minor tweaks to warning message when mod_example_dir is used

@choldgraf do you want to have a quick look at this one?

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 19, 2018

looks good to me @lesteve ... +1 to merge if you're happy w/ it!

@lesteve

This comment has been minimized.

Contributor

lesteve commented Feb 20, 2018

OK let's merge this one then!

@lesteve lesteve merged commit 78f6bec into sphinx-gallery:master Feb 20, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 91.98%)
Details
codecov/project Absolute coverage decreased by -0.05% but relative coverage increased by +8.01% compared to 4993065
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

lesteve added a commit that referenced this pull request Feb 20, 2018

Only issue warnings if mod_examples_dir is used (#307)
* remove default backreferences_dir magic default + warning
* tweak mod_examples_dir warning message
@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 21, 2018

woot! thanks for helping push this one through

@choldgraf

This comment has been minimized.

Contributor

choldgraf commented Feb 21, 2018

and thanks @NelleV for making the initial push!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment