Review Request #10165 — Created Sept. 22, 2018 and submitted Oct. 23, 2018, 11:53 a.m.
the dictionary, a None value is returned.
is much easier to read.
./tests/runtests.py and no errors resulted.
I also used the JS-tests on /js-tests/, and there were no issues.
brennie Sept. 22, 2018, 11:09 a.m.
chipx86 Sept. 26, 2018, 1:57 p.m.
brennie Sept. 28, 2018, 2:15 p.m.
In your description: `KeyError` over "KeyError"
brennie Sept. 26, 2018, 1:57 p.m.
reviewbot Sept. 26, 2018, 2:15 p.m.
reviewbot Sept. 26, 2018, 2:16 p.m.
reviewbot Sept. 26, 2018, 2:20 p.m.
reviewbot Sept. 26, 2018, 3:31 p.m.
reviewbot Sept. 26, 2018, 3:33 p.m.
brennie Sept. 28, 2018, 2:16 p.m.
Again, we can wrap this without \ as above.
Blank line between expression and block statement.
Blank line between block statements.
While you're here, blank line between these.
brennie Sept. 28, 2018, 2:17 p.m.
Lets use widget over p here.
Lets use widget over s here.
This shouldn't need the parentheses around the lambda, should it?
reviewbot Sept. 28, 2018, 2:32 p.m.
reviewbot Sept. 28, 2018, 2:34 p.m.
reviewbot Sept. 28, 2018, 2:37 p.m.
brennie Oct. 2, 2018, 8:29 p.m.
Comments should be full sentences and end in periods.
In this case just use \ to wrap to the next line since it saves a vertical line.
brennie Oct. 2, 2018, 8:31 p.m.
How about: A dictionary mapping widgets (:py:class:`~reviewboard.admin.widgets. Widget`) to whether or not they are selected (as a :py:class:`unicode`).
The summary should fit entirely on the first line. How about Return widgets sorted based on their positions.
How about: A dictionary mapping widget IDs to their ordinals.
Uppercase before lowercase, so this would be the first import.
brennie Oct. 5, 2018, 12:13 p.m.
brennie Oct. 5, 2018, 12:28 p.m.
david Oct. 15, 2018, 7:31 p.m.
david Oct. 15, 2018, 7:42 p.m.
This line can go away (the definition of a try block is to wrap something in case it fails).
david Oct. 15, 2018, 7:37 p.m.
Add a blank line here, please.
david Oct. 15, 2018, 7:35 p.m.
Sept. 22, 2018, 9:56 a.m.
~ secondary widget, should appear on the dashboard with the other widgets.
+ with the other widgets.
~ which crashed the page.
~ dictionary, a None value is returned.
~ the dictionary, a None value is returned.
I also used the JS-tests on /js-tests/, and there were no (new) issues.
~ the new widget branch, the page was able to sucessfully load the new widget.
+ page was able to sucessfully load the new widget.
Sept. 22, 2018, 10:16 a.m.
Can you rewrite your summary in the imperitive mood, i.e., as if it were a command or order?
Also can we rename y to something like widget? y is not a good variable name.
Sept. 22, 2018, 10:29 a.m.
This is a good opportunity to get comfortable with unit tests.
You'll need to write a unit test that sets up some state in the profile for the widget positions, performs a HTTP GET to the admin UI dashboard, and then checks the resulting template context to see if your fixes did what you'd expect them to do.
Sept. 22, 2018, 11:09 a.m.
Sept. 26, 2018, 2:10 p.m.
~ does not raise a KeyError, and gives the widget a position.
- page was able to sucessfully load the new widget.
Sept. 26, 2018, 2:22 p.m.
Sept. 26, 2018, 3:36 p.m.
Sept. 27, 2018, 12:02 p.m.
Please run all tests, e.g., ./tests/runtests.py. They take a few minutes, but sometimes things cascade into other suites and break things.
You are registering widgets outside of the try..finally.
Lets do it this way.
# lines 298 thru 321 without the try/finally.
We need the nested try in the finally clause because soemthing could go wrong with registering either of the widgets and we would fall into the finally clause.
We can pull out the selections into a intermediate variables to make things easier to read.
Otherwise the error message we would get when running tests (True is not False) is not helpful. This way, it will repr() print the contents of response.context['selected_primary_widgets'] to aid in debugging.
where _get_widget_selections and _sort_widgets are new functions that encapsulate the current behaviour. This refactor would go a long way towards shortening the function bodies, encapsulating repeated logic, and making this code more readable.
Sept. 28, 2018, 2:30 p.m.
+ is much easier to read.
~ ./tests/runtests.py and no errors resulted.
Sept. 28, 2018, 2:39 p.m.
Oct. 2, 2018, 9:57 a.m.
Our imports should be alphabetically sorted so this (a) should go after the reviewboard.admin.validation imports and (b) needs its import( ... ) list to be sorted.
A dictionary mapping widgets (:py:class:`~reviewboard.admin.widgets.
Widget`) to whether or not they are selected (as a :py:class:`unicode`).
The summary should fit entirely on the first line.
How about Return widgets sorted based on their positions.
A dictionary mapping widget IDs to their ordinals.
Oct. 2, 2018, 8:45 p.m.
Update documentation and style, based on Barret's suggestions.
Oct. 2, 2018, 10:35 p.m.
After these tiny nits, I'm happy with this landing!
skaefer143 Oct. 5, 2018, 12:32 p.m.
Can you add a comment here explaining why we're doing this? We both know why we wrap these in try..except and ignore the error, but future readers of this file might not grok it immediately.
Oct. 5, 2018, 12:32 p.m.
Fix documentation issues based on Barret's suggestions.
Oct. 15, 2018, 12:03 p.m.
This is a little bit too verbose (I can read the words "try", "pass", and "KeyError" in the code).
How about "If an error was encountered above, the widgets will not be registered. Ignore any errors here in that case"?
on the dashboard) and a list of the unselected widgets.
Oct. 15, 2018, 7:50 p.m.
Oct. 22, 2018, 11:31 p.m.
Oct. 23, 2018, 11:53 a.m.