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

Add sphinx-copybutton for codeblock #12528

Closed
wants to merge 9 commits into from

Conversation

thoo
Copy link

@thoo thoo commented Nov 6, 2018

Close #12512
Using sphinx-copybutton extension to add a copybutton for the code block. It should look like this:

@eamanu
Copy link
Contributor

eamanu commented Nov 6, 2018

I don't know sphinx-copybutton extension but, is this added automatically the button to whole code in the docs? or this have to added one by one?

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

The generated docs are in https://37582-843222-gh.circle-artifacts.com/0/doc/index.html they look good.

This doesn't seem to work for prompt though,
i.e. when copying https://37582-843222-gh.circle-artifacts.com/0/doc/modules/linear_model.html#lars-lasso , >>> also get copied. Meaning one would need to first click on "hide prompt" then on copy, in which case I don't really see the advantage of this over a copy past. My assumption was that copy button would only copy the code not the output.

top: 20px !important;
padding: 0px 4px 2px !important;
background: transparent no-repeat none !important;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is not included in the extension?

Copy link
Author

Choose a reason for hiding this comment

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

The naming is confusing here. The >>> prompt button is named as copybutton. I just try to re-arrange Hide prompts and output button. This is not the one I am adding.

@eamanu
Copy link
Contributor

eamanu commented Nov 6, 2018

Great!! this is a good idea. But, IMO the prompt have to be hide by default. so this way, I can copy and paste quickly without pressing the hide button. Is this a configuration?

@thoo
Copy link
Author

thoo commented Nov 7, 2018

I agree. The prompt >>> and output should be filtered out when the code block is copied. I am looking into it.

@qinhanmin2014
Copy link
Member

Thanks for the PR. The bottom doesn't seems very useful for scikit-learn. We need to first hide the prompts and outputs. What's more, many examples cannot be executed at once (e.g, we don't use print in some examples, see the Examples section in https://37582-843222-gh.circle-artifacts.com/0/doc/modules/generated/sklearn.cluster.DBSCAN.html). Feel free to improve your PR or close it (since there's another -1 from rth I guess).

@thoo
Copy link
Author

thoo commented Nov 9, 2018

This should only copy codes and exclude outputs, and prompts. @eamanu Can you test it again? It works fine on Mac but I haven't tried it on other operating systems. This extension uses clipboard.js which is independent of Flash. So I assume it will work smoothly across different platforms. I also submitted the PR for sphinx-copybutton. As of now, we have to install it from pip install -e git+https://github.com/thoo/sphinx-copybutton.git@for_scikit_learn#egg=sphinx-copybutton.

@@ -124,6 +124,7 @@ conda create -n $CONDA_ENV_NAME --yes --quiet python="${PYTHON_VERSION:-*}" \

source activate testenv
pip install sphinx-gallery
pip install -e git+https://github.com/thoo/sphinx-copybutton.git@for_scikit_learn#egg=sphinx-copybutton
Copy link
Contributor

@eamanu eamanu Nov 9, 2018

Choose a reason for hiding this comment

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

I don't know if that is a good solution.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I ended up placing css and js files under custom folder. So no python dependencies to install.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review.

@eamanu
Copy link
Contributor

eamanu commented Nov 9, 2018

@qinhanmin2014 IMHO the copy button is a little "plus". This can not take much time.

@eamanu
Copy link
Contributor

eamanu commented Nov 9, 2018

@thoo look for ci/circleci: python2 that fail

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

Thanks, the PR itself looks very nice, but the main problem remains, i.e., many of our examples cannot be executed at once (e.g, we don't use print in some examples). I'll vote +1 and we need another +1.

@thoo
Copy link
Author

thoo commented Nov 10, 2018

@qinhanmin2014 Thanks for the review. I am confused about executing at once. I usually set InteractiveShell.ast_node_interactivity = "all" in jupyter notebook if I want to see all outputs and I thought that is the reason we don't have print in some examples ??

@qinhanmin2014
Copy link
Member

I usually set InteractiveShell.ast_node_interactivity = "all" in jupyter notebook if I want to see all outputs

Thanks, I'm aware of these ways and this is why I vote +1.

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Putting each of these files in the global theme is the wrong way to do modularity and encapsulation. Rather, like doc/sphinxext/sphinx_issues.py, the code should just be included as a whole unit and activated with the usual extensions mechanism. If we need to alter it, we can do that too, leaving appropriate notes.

@@ -0,0 +1,116 @@
// Please see details at https://github.com/choldgraf/sphinx-copybutton
Copy link
Member

Choose a reason for hiding this comment

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

This is not sufficient according to https://github.com/choldgraf/sphinx-copybutton/blob/master/LICENSE:

"
The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.
"

doc/conf.py Outdated
@@ -68,7 +68,7 @@
source_suffix = '.rst'

# The encoding of source files.
#source_encoding = 'utf-8'
# source_encoding = 'utf-8'
Copy link
Member

Choose a reason for hiding this comment

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

Please do not change unrelated things. It makes your contribution harder to review and may introduce merge conflicts to other pull requests.

@jnothman
Copy link
Member

jnothman commented Nov 12, 2018 via email

@thoo
Copy link
Author

thoo commented Nov 12, 2018

@jnothman Thanks for the review. I agree it is better to be a separate extension like sphinx_issues.py .

The "hide/show the prompts and output" tooltip was tied to the whole code block and I wasn't sure it was on purpose or not when "prompts and output" button was added. It is not because of this PR. As you mention it, I will limit this tooltip only to the "prompts and output" button not to the whole code block.

@thoo
Copy link
Author

thoo commented Nov 12, 2018

@jnothman Could you re-review? I added sphinx_copybutton at sphinxext as a local extension.

@@ -0,0 +1,103 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Why can't the CSS be kept under sphinxext/sphinx_copybutton as it would be if installed as a separate package?

Copy link
Author

Choose a reason for hiding this comment

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

Sure. All the files are now under sphinxext/sphinx_copybutton.

@thoo thoo force-pushed the add_codeblock_copybutton branch 2 times, most recently from 6be1ca5 to 11fd009 Compare November 14, 2018 00:12
doc/conf.py Outdated
@@ -22,6 +22,9 @@
# is relative to the documentation root, use os.path.abspath to make it
# absolute, like shown here.
sys.path.insert(0, os.path.abspath('sphinxext'))
# Path for sphinx_copybutton ext.
sys.path.insert(0, os.path.abspath(
Copy link
Member

Choose a reason for hiding this comment

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

This should not be necessary if there is a file sphinxext/sphinx_copybutton/__init__.py

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

No I meant that like the original package, the primary code should be in sphinx_copybutton/__init__.py. if there are any worthwhile updates to the original package we would be able to simply copy it in.

@thoo
Copy link
Author

thoo commented Nov 15, 2018

Ah I see. Done.

@thoo thoo closed this Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Copy button for code block
5 participants