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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 4 months ago by Andi
Modified:
1 year, 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
2 years, 4 months ago #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 ...
2 years, 4 months ago #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 ...
2 years, 4 months ago #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 ...
2 years, 4 months ago #4
Andi Albrecht
2 years, 4 months ago #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 1274:9b9295c7fd64