Skip to content

MRG, ENH: Provide sub-extension sphinx_gallery.load_style#601

Merged
larsoner merged 5 commits intosphinx-gallery:masterfrom
mgeier:load-style
Feb 14, 2020
Merged

MRG, ENH: Provide sub-extension sphinx_gallery.load_style#601
larsoner merged 5 commits intosphinx-gallery:masterfrom
mgeier:load-style

Conversation

@mgeier
Copy link
Copy Markdown
Contributor

@mgeier mgeier commented Jan 24, 2020

As discussed in #599 (comment), this enables following usage in conf.py:

extensions = [
    'sphinx_gallery.load_style',
]

... which only loads the CSS style and sets the corresponding static directory. Nothing else.

For now, this provides a minimal solution without any configuration options.

@larsoner suggested in #599 (comment) to add configurability:

Even in the case where you want to load SG style(s), we'd still want a way to say which styles to load.

I think we don't need that yet, because for now there aren't even multiple styles, there's only one.
I think configurability can be added later (when needed), an example for a possible implementation is in 2d65445.

In case anyone is wondering what's the use case for this, here's some explanations: #317 (comment).

The "normal" use of SG should not be affected by this in any way.

@mgeier mgeier mentioned this pull request Jan 24, 2020
Merged
@codecov-io
Copy link
Copy Markdown

codecov-io commented Jan 24, 2020 •
edited
Loading

Codecov Report

Merging #601 into master will decrease coverage by 0.2%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #601      +/-   ##
==========================================
- Coverage   97.64%   97.43%   -0.21%     
==========================================
  Files          29       31       +2     
  Lines        3097     3120      +23     
==========================================
+ Hits         3024     3040      +16     
- Misses         73       80       +7
Impacted Files Coverage Δ
sphinx_gallery/tests/test_gen_gallery.py 100% <ø> (+1.11%) ⬆️
sphinx_gallery/load_style.py 100% <100%> (ø)
sphinx_gallery/tests/test_load_style.py 100% <100%> (ø)
sphinx_gallery/tests/conftest.py 96.85% <95.55%> (-0.72%) ⬇️
sphinx_gallery/tests/test_scrapers.py 97.9% <0%> (-1.4%) ⬇️
sphinx_gallery/gen_gallery.py 95.16% <0%> (-0.91%) ⬇️
sphinx_gallery/tests/test_full.py 99.34% <0%> (-0.66%) ⬇️

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 ddc7609...5ca1edb. Read the comment docs.

@mgeier mgeier mentioned this pull request Jan 26, 2020
Merged
@mgeier
Copy link
Copy Markdown
Contributor Author

mgeier commented Jan 26, 2020

In case somebody is interested in a concrete use case: spatialaudio/nbsphinx#392.

@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Jan 27, 2020

Looks reasonable to me. It would be nice not to take a coverage hit, though. Can you add a test? If it's not clear how to do it with the sphinx machinery we have in place, I can take a look and push

@mgeier
Copy link
Copy Markdown
Contributor Author

mgeier commented Jan 28, 2020

@larsoner Thanks! If you already know how a test could be added and if you have a bit of time to spare, it would be great if you could create a test.

If not, I guess I could start experimenting with creating a test file similar to sphinx_gallery/tests/test_gen_gallery.py (but much shorter).
However, to re-use some of the fixtures from there, they would have to be factored out (e.g. into sphinx_gallery/tests/conftest.py?), right?

larsoner
larsoner approved these changes Jan 29, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Test added, LGTM +1 for merge

@mgeier
Copy link
Copy Markdown
Contributor Author

mgeier commented Jan 29, 2020

Thanks a lot @larsoner for writing the test!
You don't seem to have added the test file to version control though?

@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Feb 1, 2020

@lucyleeow feel free to merge if you're happy

@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Feb 6, 2020

Or @choldgraf do you want to look?

@larsoner larsoner changed the title Provide sub-extension sphinx_gallery.load_style MRG, ENH: Provide sub-extension sphinx_gallery.load_style Feb 6, 2020
lucyleeow
lucyleeow reviewed Feb 11, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@lucyleeow lucyleeow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, just the one nitpick (which isn't from this PR but while we are amending, might as well change it). It might be worthwhile to add a note to the documentation too - maybe in advanced.rst or utils.rst (not sure which is more appropriate).

Sorry for the delay - had other work keeping me occupied.

@larsoner larsoner merged commit 0f32d04 into sphinx-gallery:master Feb 14, 2020
@larsoner
Copy link
Copy Markdown
Contributor

larsoner commented Feb 14, 2020

Thanks @mgeier !

@mgeier mgeier deleted the load-style branch February 14, 2020 15:21
@mgeier
Copy link
Copy Markdown
Contributor Author

mgeier commented Feb 14, 2020

Thanks for merging!

@mgeier mgeier mentioned this pull request Mar 24, 2020
Closed
This was referenced Apr 8, 2021
Closed
Merged
@larsoner larsoner mentioned this pull request Sep 13, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lucyleeow lucyleeow lucyleeow left review comments

@larsoner larsoner larsoner approved these changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mgeier @codecov-io @larsoner @lucyleeow