Summary

RB Close

Review Request #3595 — Created Nov. 30, 2012 and submitted Jan. 9, 2013, 6:17 p.m.

Information

Owner

jsintal

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

RB Close

Allows the user to submit a command 'rb close <rid>' to mark a review request as either 'discarded' or 'submitted'.

Testing Done

On local dev server, tried several combinations of the command and viewed the review request to ensure it match what was desired by the command.

Working cases:
rb close <rid> 
rb close <rid> --description='some desc'
rb close <rid> --type='submitted' 
rb close <rid> --type='submitted' --description='some desc'
rb close <rid> --type='discarded' 
rb close <rid> --type='discarded' --description='some desc'

Bad input cases:
rb close <bad rid>
rb close <rid> --type='bad type' 

Retested with rb-general changes (removal of extra options)

Issues

Description	From	Last Updated
@smacleod : can we rip this out so I don't have to deal with all the coupling related to self.tool …	JS jsintal	Dec. 2, 2012, 1:57 p.m.
Any mentors know how to properly format this line?	JS jsintal	Dec. 2, 2012, 2:21 p.m.
"discarded"	chipx86	Dec. 4, 2012, 3:28 p.m.
"user can provide"	chipx86	Dec. 4, 2012, 3:28 p.m.
Shouldn't be needed.	chipx86	Dec. 4, 2012, 3:48 p.m.
One line, and "number" isn't right. Just say "Returns the review request resource for the given ID."	chipx86	Dec. 4, 2012, 3:30 p.m.
Where does this come from? I think you mean: sys.exit(1)	chipx86	Dec. 4, 2012, 3:30 p.m.
type_ is kind of weird. How about close_type?	chipx86	Dec. 4, 2012, 3:31 p.m.
A nice Pythonism you can use instead is: if close_type not in (SUBMITTED, DISCARDED):	chipx86	Dec. 4, 2012, 3:32 p.m.
Should be a period after "type"	chipx86	Dec. 4, 2012, 3:33 p.m.
close_type	chipx86	Dec. 4, 2012, 3:33 p.m.
No need for parens.	chipx86	Dec. 4, 2012, 3:34 p.m.
No need for parens.	chipx86	Dec. 4, 2012, 3:34 p.m.
Slight alignment problem here I think: Either request.update(data={'status': type_, 'description': self.options.description}) Or request.update( data={ 'status': type_, 'description': self.options.description } ) …	SL slchen	Dec. 4, 2012, 3:28 p.m.
Alphabetical.	chipx86	Dec. 4, 2012, 3:35 p.m.
lowercase the O, I believe.	mike_conley	Dec. 4, 2012, 8:27 p.m.
I'm starting to see this pattern repeat itself. Are we sure this isn't something we need in the parent class?	mike_conley	Dec. 4, 2012, 8:41 p.m.
Nit: indented too far	mike_conley	Dec. 4, 2012, 8:28 p.m.

Your diff doesn't seem to have posted correctly.

Status: Closed (submitted)

Status: Re-opened

Change Summary:

Includes the rbclose.py file.

Diff:

Revision 2 (+104 -43)

Show changes

	setup.py
	rbtools/commands/rbclose.py

rbtools/commands/rbclose.py (Diff revision 2)

Show all issues

@smacleod : can we rip this out so I don't have to deal with all the coupling related to self.tool (SCM stuff)?

Change Summary:

Reinserted SCM related stuff (superfluous options, methods, and etc) for the scan_server_url() method so it doens't crash if it goes if it tries to use the SCM tool.

Diff:

Revision 3 (+151 -43)

Show changes

	setup.py
	rbtools/commands/rbclose.py

Change Summary:

- refactored according to the changes in base commadnd
- pep8 style fixes
- logic added to detect if a request had been already marked as 'discarded' or 'submitted'

Diff:

Revision 4 (+116 -43)

Show changes

	setup.py
	rbtools/commands/rbclose.py

rbtools/commands/rbclose.py (Diff revisions 3 - 4)

Show all issues

Any mentors know how to properly format this line?

mike_conley Dec. 2, 2012, 2:05 p.m.

request = \
    self.root_resource.get_review_requests().get_item(request_id)

Does that give you enough?

jsintal Dec. 2, 2012, 2:21 p.m.

Nice, that works, my pep8 compiler doesn't complain. I was also thinking of:

request = self.root_resource.get_review_requests() \
                      .get_item(request_id)


But my pep8 compiler complains, saying : 'rbclose.py:96:23: E127 continuation line over-indented for visual indent'

Change Summary:

Changed the way commands are branched from other features (rb-general). Ex.: http://pastie.org/5468764

Diff:

Revision 5 (+1)

Show changes

setup.py

Change Summary:

Changed the way commands are branched from other features (rb-general). Ex.: http://pastie.org/5468764

Change Summary:

rebased

Testing Done:

		On local dev server, tried several combinations of the command and viewed the review request to ensure it match what was desired by the command.

		Working cases:
		rb close <rid>
		rb close <rid> --description='some desc'
		rb close <rid> --type='submitted'
		rb close <rid> --type='submitted' --description='some desc'
		rb close <rid> --type='discarded'
		rb close <rid> --type='discarded' --description='some desc'

		Bad input cases:
		rb close <bad rid>
		rb close <rid> --type='bad type'
	+
	+	Retested with rb-general changes (removal of extra options)

Diff:

Revision 6 (+81 -44)

Show changes

	setup.py
	rbtools/commands/rbclose.py

Change Summary:

Rebased on using  the gettatr(options, field, default method for ALL

Diff:

Revision 7 (+81 -44)

Show changes

	setup.py
	rbtools/commands/rbclose.py

rbtools/commands/rbclose.py (Diff revision 7)

Show all issues

Slight alignment problem here I think: 

Either

request.update(data={'status': type_,
                     'description': self.options.description})

Or

request.update(
    data={
              'status': type_,
              'description': self.options.description
         }
    )

?

chipx86 Dec. 4, 2012, 1:01 a.m.

Sort of the latter, but do:

request.update(data={
    'status': type_,
    'description': self.options.description,
}))

rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
"discarded"
```
rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
"user can provide"
```

rbtools/commands/rbclose.py (Diff revision 7)

Show all issues

Shouldn't be needed.

rbtools/commands/rbclose.py (Diff revision 7)

Show all issues

One line, and "number" isn't right. Just say "Returns the review request resource for the given ID."

rbtools/commands/rbclose.py (Diff revision 7)

Show all issues

Where does this come from? I think you mean:

sys.exit(1)

rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
type_ is kind of weird. How about close_type?
```

rbtools/commands/rbclose.py (Diff revision 7)

Show all issues

A nice Pythonism you can use instead is:

if close_type not in (SUBMITTED, DISCARDED):

rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
Should be a period after "type"
```
rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
close_type
```
rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
No need for parens.
```
rbtools/commands/rbclose.py (Diff revision 7)
Show all issues
```
No need for parens.
```
setup.py (Diff revision 7)
Show all issues
```
Alphabetical.
```

John: I see your issues raised by Christian have been fixed. Do you have an updated patch?

Change Summary:

Addressed all issues

Diff:

Revision 8 (+74 -44)

Show changes

	setup.py
	rbtools/commands/rbclose.py

rbtools/commands/rbclose.py (Diff revision 8)
Show all issues
```
lowercase the O, I believe.
```

rbtools/commands/rbclose.py (Diff revision 8)

Show all issues

I'm starting to see this pattern repeat itself. Are we sure this isn't something we need in the parent class?

smacleod Dec. 4, 2012, 8:33 p.m.

This is done because of the way options are currently defined. When creating the options on the class, the config has yet to be parsed, so we can't use default values from it.

I'm working on a new way to handle options which fixes this.

You can drop this issue John.

rbtools/commands/rbclose.py (Diff revision 8)
Show all issues
```
Nit: indented too far
```

setup.py (Diff revision 8)

Y'know, when all of these commands land, this is all gonna conflict right here. :)

jsintal Dec. 4, 2012, 8:29 p.m.

Yikes! Is there anything I can do to avoid that?

chipx86 Dec. 4, 2012, 10:23 p.m.

The merges won't be terrible. You'll just have to make the result look correct.

Change Summary:

O changed to o for options

Diff:

Revision 9 (+74 -44)

Show changes

	setup.py
	rbtools/commands/rbclose.py

Change Summary:

Minor bug fix

Diff:

Revision 10 (+75 -44)

Show changes

	setup.py
	rbtools/commands/rbclose.py

FWIW, looks reasonable to me, and works on Linux against RB 1.7.1.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to api (cecc50c00b50637b5ded49266640aa7c3e7defcc).