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

[Review] Code refactoring, block execution of scripts #36

Merged
merged 88 commits into from Jul 31, 2015

Conversation

Projects
None yet
5 participants
@Titan-C
Member

Titan-C commented Jun 7, 2015

I'm still unsure which the final goal of this clean up is. By now it breaks up some statements into simpler functions.
One big advantage is that now the re-execution of a example script is evaluated a lot earlier and one saves a lot of if..else, so less branching and less deeper indentation.

  • docstrings for this functions
  • try to test on them
# We need to execute the code
print('plotting %s' % fname)
t0 = time()
import matplotlib.pyplot as plt

This comment has been minimized.

@Titan-C

Titan-C Jun 8, 2015

Member

@lesteve
I'm actually moving it out of here. I asked in scikit-learn if there was a good reason to keep this import so nested in the function and not moving it to the top of the file, no answer yet.

This comment has been minimized.

@lesteve

lesteve Jun 8, 2015

Contributor

Makes sense, I misread the diff.

This comment has been minimized.

@jnothman

jnothman Jun 8, 2015

Contributor

Its purpose would be to not require matplotlib to compile documentation
that used the gallery without plotting...

On 8 June 2015 at 18:26, Loïc Estève notifications@github.com wrote:

In sphinxgallery/gen_rst.py
#36 (comment)
:

@@ -310,81 +339,57 @@ def execute_script(image_dir, thumb_file, image_fname, base_image_name,
if os.path.exists(time_path):
time_elapsed = float(open(time_path).read())

  • if not os.path.exists(first_image_file) or \
  •   os.stat(first_image_file).st_mtime <= os.stat(src_file).st_mtime:
    
  •    # We need to execute the code
    
  •    print('plotting %s' % fname)
    
  •    t0 = time()
    
  •    import matplotlib.pyplot as plt
    

Makes sense, I misread the diff.


Reply to this email directly or view it on GitHub
https://github.com/sphinx-gallery/sphinx-gallery/pull/36/files#r31894523
.

This comment has been minimized.

@GaelVaroquaux

GaelVaroquaux via email Jul 13, 2015

Contributor

This comment has been minimized.

@Titan-C

Titan-C Jul 13, 2015

Member

But I think it misses the point of not requiring matplotlib if we call it anyway to set the backend Agg before everything.

@Titan-C Titan-C force-pushed the Titan-C:refactor_split branch from cc1a0f0 to f20d618 Jun 13, 2015

Titan-C added some commits Jun 13, 2015

No need of time & stdout cache files as script re-execution is only t…
…rigerred on file update else no rst file is rewritten
def codestr2rst(codestr):
"""Return reStructuredText code block from code string"""
code_directive = "\n.. code-block:: python\n\n"
indented_block = ' ' + codestr.replace('\n', '\n ')

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

You can use textwrap.indent here. I think this could be used in one or two other places in this file.

def extract_intro(filename):
""" Extract the first paragraph of module-level docstring. max:95 char"""
first_text = split_code_and_text_blocks(filename)[0][-1]

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

It is a bit of a shame, that split_code_and_text_blocks is called again here to get the docstring given that you have already done the necessary work in generate_file_rst.

One way to fix this would be to return the docstring from generate_rst and pass it to make extract_info which would take a docstring instead of a filename. Better suggestions welcome!

first_image_file = image_file.format(1)
needs_replot = (not os.path.exists(first_image_file) or
os.stat(first_image_file).st_mtime <= os.stat(src_file).st_mtime)

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

PEP8 under-indentation

if not os.path.exists(target_dir):
os.makedirs(target_dir)
sorted_listdir = [fname for fname in sorted(os.listdir(src_dir))
if fname.endswith('py')]

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

PEP8

/%s/%s\n""" % (target_dir, fname[:-3])

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

PEP8 too many blank lines

return fhindex
def execute_script(code_block, example_globals, image_path, fig_count, src_file):

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

PEP8 line too long.

Pro tip: take some time setting up PEP8 checks in your editor of choice, it will be worth the investment in the long run.

first_par = ((first_par[:95] + '...')
if len(first_par) > 95 else first_par)
else:
raise ValueError("Missing first paragraph."

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

You might want a slightly better error message here. For example mentioning that you need a title (as a rst header) and a short explanation paragraph or something better.

time_elapsed += rtime
# Single example style
if len(script_blocks) == 2:

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

script_blocks length is not going to change in the for blabel, bcontent in script_blocks loop. You should put that in a nicely named boolean variable outside of the loop, for example:

is_example_notebook_like = len(script_blocks) > 2

for blabel, bcontent in script_blocks:
    ...
    if has_example_notebook_style:
        ...
    else:
        ...

This way you can even get rid of the comments

example_globals = {}
fig_count = 0
for blabel, bcontent in script_blocks:
if blabel == 'code':

This comment has been minimized.

@lesteve

lesteve Jul 20, 2015

Contributor

slight niggle: you can take that out of the loop:

code_blocks = [label, content for label, content in script_blocks if label == 'code']
for blabel, bcontent in code_blocks:
    ...

Now you can even use len(code_blocks) > 1 as the is_example_notebook_like condition which seems a tad less magical.

This comment has been minimized.

@lesteve

lesteve Jul 30, 2015

Contributor

Actually I was wrong, I didn't see the else clause below ...

@jberlanga jberlanga referenced this pull request Jul 22, 2015

Closed

Add Sphinx Plot Gallery #23

lesteve added some commits Jul 30, 2015

MAINT fix typo coment -> comment
and some PEP8 fixes
MAINT code clarification
for simple examples vs notebook-like example
MAINT compatibility layer for textwrap.indent for python 2
and fix missing argument in indent function
More robust separation of docstring and of the file
Amend test because a newline was missing in the expected result. It
should have no impact on the visual result.
Use get_docstring_and_rest to get example tooltip
rather than split_code_and_text_blocks since we only need the docstring.
Also rename variable and improve error message

@lesteve lesteve force-pushed the Titan-C:refactor_split branch from 33b06a3 to e532187 Jul 30, 2015

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jul 31, 2015

OK I pushed a few commits to address my remaining comments so I am going to merge this one as is.

lesteve added a commit that referenced this pull request Jul 31, 2015

Merge pull request #36 from Titan-C/refactor_split
[Review] Code refactoring, block execution of scripts

@lesteve lesteve merged commit c3b3144 into sphinx-gallery:master Jul 31, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lesteve

This comment has been minimized.

Contributor

lesteve commented Jul 31, 2015

@Titan-C, great stuff btw, thanks!

@agramfort

This comment has been minimized.

Contributor

agramfort commented Jul 31, 2015

@Titan-C, great stuff btw, thanks!

+1

Reply to this email directly or view it on GitHub.

@GaelVaroquaux

This comment has been minimized.

Contributor

GaelVaroquaux commented Jul 31, 2015

@Titan-C Titan-C referenced this pull request Jul 31, 2015

Closed

Path class #35

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jul 31, 2015

@Titan-C BTW if you have five minutes, just double-check that nothing subtle has broken because of my recent commits: http://sphinx-gallery.readthedocs.org/en/latest/

@Titan-C

This comment has been minimized.

Member

Titan-C commented Jul 31, 2015

I think it looks fine.

@lesteve

This comment has been minimized.

Contributor

lesteve commented Jul 31, 2015

I think it looks fine.

Good to know, thanks for double-checking.

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