Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(826)

Issue 5449109: Move FetchError to codereview.exceptions. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
12 years, 3 months ago by Andi
Modified:
11 years, 8 months ago
Reviewers:
techtonik, GvR, M-A
CC:
codereview-list_googlegroups.com
Visibility:
Public.

Description

This is the first of a series of patches to remove the cyclic import engine <-> models. My first approach was just to move the constants to settings.py, but it turned out that some more refactoring is needed to completely avoid cyclic imports - and that it is a nice opportunity for some cleanups. Here are the steps I'm about to do: - Move FetchError to a new module codereview.exceptions (this patch). - Move two helper functions ToText and UnifyLinebreaks to a new module codereview.utils. Both are used in views.py and models.py too. - Replace engine.FetchBase by models.Patch.fetch_base since that's the only place where we actually need FetchBase. After this refactoring engine.py should only be responsible for rendering. - Move constants from engine.py to settings.py - and finally clean up relative imports I splitted the whole change to make it readable. -- Andi

Patch Set 1 #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+39 lines, -17 lines) Patch
M codereview/engine.py View 1 chunk +1 line, -4 lines 0 comments Download
A codereview/exceptions.py View 1 chunk +23 lines, -0 lines 3 comments Download
M codereview/models.py View 3 chunks +5 lines, -4 lines 0 comments Download
M codereview/views.py View 7 chunks +10 lines, -9 lines 0 comments Download

Messages

Total messages: 5
Andi Albrecht
12 years, 3 months ago (2011-12-07 04:47:28 UTC) #1
techtonik
Cool. LGTM. http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py File codereview/exceptions.py (right): http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py#newcode23 codereview/exceptions.py:23: """Exception raised when fetching of base files ...
12 years, 3 months ago (2011-12-07 08:16:26 UTC) #2
Andi Albrecht
http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py File codereview/exceptions.py (right): http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py#newcode23 codereview/exceptions.py:23: """Exception raised when fetching of base files fails.""" On ...
12 years, 3 months ago (2011-12-07 19:54:05 UTC) #3
M-A
lgtm as long as ./tools/run_pylint.sh still pass. http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py File codereview/exceptions.py (right): http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py#newcode23 codereview/exceptions.py:23: """Exception raised ...
12 years, 3 months ago (2011-12-07 20:27:42 UTC) #4
Andi Albrecht
12 years, 3 months ago (2011-12-08 11:40:33 UTC) #5
On Wed, Dec 7, 2011 at 9:27 PM,  <maruel@chromium.org> wrote:
> lgtm as long as ./tools/run_pylint.sh still pass.

pylint passes for all changes in this refactoring :)
the goal is to enable R0401 "cyclic import" and W0403 "relative import".

>
>
>
> http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py
> File codereview/exceptions.py (right):
>
>
http://codereview.appspot.com/5449109/diff/1/codereview/exceptions.py#newcode23
> codereview/exceptions.py:23: """Exception raised when fetching of base
> files fails."""
> On 2011/12/07 19:54:05, Andi Albrecht wrote:
>>
>> On 2011/12/07 08:16:27, techtonik wrote:
>> > I feel like either name or description should be modified to clarify
>
> if it is
>>
>> a
>> > generic exception or specific one for base files only ("..for
>
> example, when
>>
>> > fetching base files.").
>
>
>> Done.
>
>
> FYI, you forgot to upload another patchset.

oh, sorry...

>
> http://codereview.appspot.com/5449109/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b