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

fix utils.template.render_templatefile() bug +test #1212

Merged
merged 2 commits into from Aug 26, 2015

Conversation

@Digenis
Copy link
Member

@Digenis Digenis commented May 6, 2015

I fixed a potential bug (untriggered so far because of the .py extension of the template files)
but while writing the test, it ended taking up most of the changes done in this PR.
Please comment whether I should just push the bugfix.

The potential bug is that .rstrip() in utils.template.render_templatefile()
is used like it is expected to strip a string, not a character class
(think of using re.sub(r'[\.tmpl]*$, '', path) instead of re.sub(r'\.tmpl$', '', path).
On filepaths that always end with '.py.tmpl', the problem goes unnoticed.

Also, shouldn't there be some expectation or even an assertion
that the template files have the '.tmpl' extension?

@kmike
Copy link
Member

@kmike kmike commented May 6, 2015

@Digenis - a good catch about rstrip, +1 to fix it.

I'm not a fan of mock tests though - they are unreadable, and they may require a rewrite if the internal implementation changes. But some tests may be better than no tests :)

Maybe we should just switch to https://github.com/audreyr/cookiecutter to reduce maintenance burden. It is well-supported, provide more features and already works in Python 3.

@@ -10,7 +10,7 @@ def render_templatefile(path, **kwargs):

content = string.Template(raw).substitute(**kwargs)

with open(path.rstrip('.tmpl'), 'wb') as file:
with open(re.sub(r'\.tmpl$', '', path), 'wb') as file:

This comment has been minimized.

@dangra

dangra May 7, 2015
Member

I prefer to use simple string functions instead of regex whenever I can, my suggestion is to use path.rpartition('.')[0] if this is about striping the template extension only.
In any case it looks good to me.

@Digenis
Copy link
Member Author

@Digenis Digenis commented May 7, 2015

@Digenis Digenis force-pushed the Digenis:test-fix-render_template_file branch from 8b8c052 to 85b80b6 May 7, 2015
@Digenis
Copy link
Member Author

@Digenis Digenis commented May 7, 2015

@dangra I would use .rpartition if there was some assurance about the filename .tmpl extension.
Since there is none, I switched to something equivalent of the previous regex
(and to what I think was intended on the original expression)

What I would do in order of preference is:
either always pass the path to the output file and let the func append the .tmpl extension where needed,
or assert inside the function body that the extension is there.

Should I go for the first one?

@@ -10,7 +10,8 @@ def render_templatefile(path, **kwargs):

content = string.Template(raw).substitute(**kwargs)

with open(path.rstrip('.tmpl'), 'wb') as file:
render_path = path[:len('.tmpl')] if path.endswith('.tmpl') else path
with open(render_path) as file:

This comment has been minimized.

@dangra

dangra May 7, 2015
Member

missing w mode.

@@ -10,7 +10,8 @@ def render_templatefile(path, **kwargs):

content = string.Template(raw).substitute(**kwargs)

with open(path.rstrip('.tmpl'), 'wb') as file:
render_path = path[:len('.tmpl')] if path.endswith('.tmpl') else path

This comment has been minimized.

@dangra

dangra May 7, 2015
Member

>>> path = 'templ.tmpl'
>>> path[:len('.tmpl')]
'templ'
>>> path = 'footempl.tmpl'
>>> path[:len('.tmpl')]
'foote'
>>> path[:-len('.tmpl')]
'footempl'
@dangra
Copy link
Member

@dangra dangra commented May 7, 2015

@dangra I would use .rpartition if there was some assurance about the filename .tmpl extension.
Since there is none, I switched to something equivalent of the previous regex
(and to what I think was intended on the original expression)

What I would do in order of preference is:
either always pass the path to the output file and let the func append the .tmpl extension where needed,
or assert inside the function body that the extension is there.

Should I go for the first one?

+1 to keep same behavior. Ignore the rpartition suggestion.

@Digenis Digenis force-pushed the Digenis:test-fix-render_template_file branch from 85b80b6 to 4ffd278 May 7, 2015
@Digenis
Copy link
Member Author

@Digenis Digenis commented May 7, 2015

What about striping the extension in the argument to the render_templatefile() call?
(as I said above)

@dangra
Copy link
Member

@dangra dangra commented May 7, 2015

by stripping in the caller you are changing the function public interface and that will lead to more discussions for a bugfix that is very simple to fix within the function.

@Digenis
Copy link
Member Author

@Digenis Digenis commented May 7, 2015

ok
I had greped just two calls to it in the codebase,
this is why I suggested so (because of the easiness).

@Digenis
Copy link
Member Author

@Digenis Digenis commented May 9, 2015

Is there something left to do (in the scope of this PR)?

@dangra
Copy link
Member

@dangra dangra commented May 9, 2015

what is the point of mocking open() when the test can use a real file?

@Digenis
Copy link
Member Author

@Digenis Digenis commented May 10, 2015

I thought this is how people mostly test side-effects.
Is there a different convention in scrapy?

@dangra
Copy link
Member

@dangra dangra commented May 15, 2015

Is there a different convention in scrapy?

for things that creates actual files we let them create temporal files.

@kmike
Copy link
Member

@kmike kmike commented Aug 24, 2015

+1 to create a real file; in my experience tests without mocks are easier to read and less likely to break (as they don't rely on implementation details). You can also pause the test and check if correct files exists manually. The downside is execution speed (creating/removing real files is slower), but it shouldn't be a problem in this test case.

@Digenis
Copy link
Member Author

@Digenis Digenis commented Aug 26, 2015

OK, fair enough, I'll use the test_dir.
To rebase or not to rebase?

@dangra
Copy link
Member

@dangra dangra commented Aug 26, 2015

+1 to rebase

@codecov-io
Copy link

@codecov-io codecov-io commented Aug 26, 2015

Current coverage is 82.23%

Merging #1212 into master will not affect coverage as of d151499

@@            master   #1212   diff @@
======================================
  Files          165     165       
  Stmts         8172    8173     +1
  Branches      1133    1133       
  Methods          0       0       
======================================
+ Hit           6720    6721     +1
  Partial        262     262       
  Missed        1190    1190       

Review entire Coverage Diff as of d151499

Powered by Codecov. Updated on successful CI builds.

@Digenis Digenis force-pushed the Digenis:test-fix-render_template_file branch from cc9ac0b to b98ba53 Aug 26, 2015
@Digenis
Copy link
Member Author

@Digenis Digenis commented Aug 26, 2015

Used tempfile and rebased.
Waiting for feedback.

@dangra
Copy link
Member

@dangra dangra commented Aug 26, 2015

looks good, squash?

@Digenis Digenis force-pushed the Digenis:test-fix-render_template_file branch from b98ba53 to 56b3cf0 Aug 26, 2015
@Digenis
Copy link
Member Author

@Digenis Digenis commented Aug 26, 2015

squahsed

dangra added a commit that referenced this pull request Aug 26, 2015
fix utils.template.render_templatefile() bug +test
@dangra dangra merged commit 71bd79e into scrapy:master Aug 26, 2015
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@dangra
Copy link
Member

@dangra dangra commented Aug 26, 2015

thanks!

@Digenis Digenis deleted the Digenis:test-fix-render_template_file branch Oct 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.