Skip to content

Conversation

@padix-key
Copy link
Contributor

This PR removes the whitespace stripping from code output. Therefore, indent at the beginning of the output will be retained.

Example:
With this PR:

  A B
A 1 2
B 3 4

Without this PR:

A B
A 1 2
B 3 4

Is there any case, where stripping is neccessary?

@codecov-io
Copy link

codecov-io commented Apr 16, 2019

Codecov Report

Merging #475 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
+ Coverage   96.28%   96.29%   +0.01%     
==========================================
  Files          29       29              
  Lines        2636     2647      +11     
==========================================
+ Hits         2538     2549      +11     
  Misses         98       98
Impacted Files Coverage Δ
sphinx_gallery/tests/test_gen_rst.py 98.47% <100%> (+0.09%) ⬆️
sphinx_gallery/gen_rst.py 98.25% <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 83ea1ac...fd7f303. Read the comment docs.

@larsoner
Copy link
Contributor

Can you add a small test to test_gen_rst.py that would fail on master but pass on this PR?

Is there any case, where stripping is neccessary?

I would worry that an indentation of the first line could be interpreted as the intended indentation "level" for the code block. But maybe as long as they are all indented by at least 4 spaces (as they will be) it won't matter if the first one is indented by, say, 8 spaces. Have you tried it / does it work?

If it doesn't, there is probably some RST trick to make it work.

One way it probably won't is if there is only a single line, and there is starting whitespace. In this case, we could probably do something fancy with unicode spacing to force it to actually space it out, but this might be overkill.

@padix-key
Copy link
Contributor Author

padix-key commented Apr 17, 2019

I added the requested test.

I would worry that an indentation of the first line could be interpreted as the intended indentation "level" for the code block. But maybe as long as they are all indented by at least 4 spaces (as they will be) it won't matter if the first one is indented by, say, 8 spaces. Have you tried it / does it work?

I tested this and I found that this is not a problem.

One way it probably won't is if there is only a single line, and there is starting whitespace. In this case, we could probably do something fancy with unicode spacing to force it to actually space it out, but this might be overkill.

I think indentation for a single output line is not necessary, since indentation is usually used to align multiple lines to each other.

@padix-key
Copy link
Contributor Author

Could you have a look, what went wrong here? The failure is in test_full.py, a file that I have not touched and seems to happen on Windows at Python 2.7 only. I do not quite understand how my added test caused this error.

Copy link
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.

The AppVeyor error looks spurious, I restarted it. LGTM +1 for merge

@larsoner larsoner changed the title Output from code execution is not stripped FIX: Output from code execution is not stripped Apr 17, 2019
@larsoner larsoner added the bug label Apr 17, 2019
@larsoner larsoner merged commit 40d7e65 into sphinx-gallery:master May 10, 2019
@larsoner
Copy link
Contributor

Thanks @padix-key !

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants