Summary

Delete an unused variable in UsernameColumn.py

Review Request #9495 — Created Jan. 19, 2018 and submitted Feb. 12, 2018, 11:32 a.m.

Information

Owner

ridum

Repository

Review Board

Branch

release-3.0.x

Bugs

4630

Depends On

Commit

3ff35c2...

Reviewers

Groups

reviewboard, students

People

Description

The user_url variable is dead code that accidentally wasn't removed
in 43957396.
This patch removes the unused variable.

Testing Done

Manual: * Linted user_url and no longer saw an unused variable.

Automated: * Ran unit tests.

Issues

Description	From	Last Updated
Your summary should be a high-level overview of your change. It shouldn't require looking up the bug number.	brennie	Jan. 19, 2018, 12:13 p.m.
4630 should go in the bugs field.	brennie	Jan. 19, 2018, 12:13 p.m.
You'll need to fill out the testing done field.	brennie	Jan. 19, 2018, 12:15 p.m.
This change looks fine, but I have one minor nit regarding the Description: due to the recent update If you're …	mike_conley	Jan. 20, 2018, 7:50 a.m.
You can mark things as code using backquotes, e.g: The `user_url`	brennie	Jan. 21, 2018, 6:40 a.m.
Mind wrapping your description at 72chars?	brennie	Jan. 21, 2018, 6:41 a.m.
Mind formatting the testing done as: * Ran unit tests. * Linted the file and no longer saw an unused …	brennie	Jan. 21, 2018, 6:41 a.m.

flake8 passed.

JSHint passed.

You will want to give our guide on writing on change descriptions a read.

Show all issues

Your summary should be a high-level overview of your change. It shouldn't require looking up the bug number.

Show all issues
```
4630 should go in the bugs field.
```

Show all issues

You'll need to fill out the testing done field.

Summary:

-	fix 4630
+	delete unused varibale in UsernameColumn.py

Testing Done:

	+	Manual:
	+	Using find to global search the dependency of "user_url"
	+	Automated:
	+	running runtest.py

Bugs:

+	4630

Show all issues

This change looks fine, but I have one minor nit regarding the Description:

due to the recent update

If you're referring to a particular update, you might want to mention which one.

I just took a quick look at reviewboard/datagrids/columns.py, and it looks like it was this change:

https://github.com/reviewboard/reviewboard/commit/439573964de820cb8663ba95db50ad02d9226794#diff-2a6f9481a47b04e51de14d94fc1999d1

So perhaps you could change this to:

The "user_url" variable is dead code that accidentally wasn't removed in 43957396. This patch removes the unused variable.

Description:

~		"user_url" assignment in UsernameColumn.render_data is unused due to the recent update. Removed it from the code
	~	The "user_url" variable is dead code that accidentally wasn't removed in 43957396. This patch removes the unused variable.

A few last small requests about the summary/description:

Please start your summary with a capital letter ("Delete ...")
You have a typo in the summary (varibale -> variable)
The summary should be "Delete an unused..."
Please wrap the description to 70 columns.

RI

ridum Jan. 20, 2018, 8:55 a.m.
```
done :P thanks for pointing out
```

This looks good to me. Thanks!

mike_conley Jan. 20, 2018, 8:43 a.m.

Whoops - revoking ship-it; I didn't notice that David had some follow-ups.

Summary:

-	delete unused varibale in UsernameColumn.py
+	Delete an unused variable in UsernameColumn.py

Description:

~		The "user_url" variable is dead code that accidentally wasn't removed in 43957396. This patch removes the unused variable.
	~	The "user_url" variable is dead code that accidentally
	+	wasn't removed in 43957396.
	+	This patch removes the unused variable.

Show all issues

You can mark things as code using backquotes, e.g:

The `user_url`

Show all issues

Mind wrapping your description at 72chars?

Show all issues

Mind formatting the testing done as:

* Ran unit tests.
* Linted the file and no longer saw an unused variable.

Description:

~		The "user_url" variable is dead code that accidentally
~		wasn't removed in 43957396.
	~	The `user_url` variable is dead code that accidentally wasn't removed
	~	in 43957396.
		This patch removes the unused variable.

Testing Done:

~		Manual:
~		Using find to global search the dependency of "user_url"
~		Automated:
~		running runtest.py
	~	Manual:
	~	* Linted `user_url` and no longer saw an unused variable.
	~	Automated:
	~	* Ran unit tests.

Testing Done:

		Manual:
~		* Linted `user_url` and no longer saw an unused variable.
~		Automated:
	~	* Linted `user_url` and no longer saw an unused variable.
	~
	+	Automated:
		* Ran unit tests.

Ship it!

```
Looks good to me! Thanks!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (8a8fd3e)