Summary

`rbt post` command uses API's DiffValidation endpoint before upload

Review Request #6407 — Created Oct. 6, 2014 and submitted Oct. 19, 2014, 1:13 p.m.

Information

Owner

asalahli

Repository

RBTools

Branch

master

Bugs

Depends On

Commit

a288b2c...

Reviewers

Groups

rbtools, students

People

Description

Before updating a review request or creating a new one, `rbt post`
command validates the diff file and terminates if it does not pass
validation. This prevents the command from creating empty review
requests when the supplied diff file is invalid.

Testing Done

Tested rbt post with:

1. Empty diff (error 219)

2. Diff from a different repository (error 207)

3. Non-diff file as a diff (error 224)

4. Valid diff (success)
using:

1. Git

2. Mercurial
Also tested base_dir with a subversion reporitory.

Issues

Description	From	Last Updated
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	Oct. 6, 2014, 1:01 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Oct. 6, 2014, 1:01 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Oct. 6, 2014, 1:01 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Oct. 6, 2014, 1:01 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Oct. 6, 2014, 1:01 a.m.
Col: 11 E111 indentation is not a multiple of four	reviewbot	Oct. 6, 2014, 1:01 a.m.
Can you move this to come after DiffResource? You just stuck it in between DiffListResource and DiffResource.	david	Oct. 10, 2014, 10:53 p.m.
The resource isn't actually a "List", it's just a bug in the API docs. Let's just document it here as …	SM smacleod	Oct. 10, 2014, 10:51 p.m.
This should have a comment mentioning the TODO of unifying this and DiffListResource.update_diff (and probably a correspending comment in update_diff)	david	Oct. 10, 2014, 10:52 p.m.
You should avoid breaking lines with \\ whenever possible. This should probably be written: RESOURCE_MAP['application/vnd.reviewboard.org.diff-validation'] = ( DiffValidationResource)	SM smacleod	Oct. 10, 2014, 10:51 p.m.
You need to pass in the parent diff and base dir, if they exist. Parent diffs can be tested by …	david	Oct. 12, 2014, 1:59 p.m.
Can you make sure that everything here uses single quotes?	david	Oct. 17, 2014, 4:38 p.m.
Can you use single quotes for these strings?	david	Oct. 17, 2014, 4:38 p.m.

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py

rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```
rbtools/commands/post.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 11
 E111 indentation is not a multiple of four
```

Commit:

b90e11b8ad13e2971d8a301ca53f04ee8f879614

eb61604ce95a7a9f25aeef9914c4528dd39f5cdc

Diff:

Revision 2 (+46)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py

If this is more or less complete, you should remove the 'WIP' tag from the summary.

I'd like to see more testing with additional version control systems (and some testing of parent diffs).

rbtools/api/resource.py (Diff revision 2)

The issue has been resolved. Show all issues

Can you move this to come after DiffResource? You just stuck it in between DiffListResource and DiffResource.

asalahli Oct. 10, 2014, 10:52 p.m.

I moved it to the end of file, since ValidateDiff resource comes last in Resource Tree page of the documentation. https://www.reviewboard.org/docs/manual/dev/webapi/2.0/resources/resource-tree/

rbtools/api/resource.py (Diff revision 2)

The issue has been resolved. Show all issues

This should have a comment mentioning the TODO of unifying this and DiffListResource.update_diff (and probably a correspending comment in update_diff)

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

The issue has been resolved. Show all issues

You need to pass in the parent diff and base dir, if they exist.

Parent diffs can be tested by making two commits on git, and then running 'rbt post HEAD' with the latest commit checked out. The base dir can be tested by connecting an SVN repository and posting a change against it.

rbtools/api/resource.py (Diff revision 2)

The issue has been resolved. Show all issues

The resource isn't actually a "List", it's just a bug in the API docs. Let's just document it here as The Validate Diff resource specific base class.

rbtools/api/resource.py (Diff revision 2)

The issue has been dropped. Show all issues

You should avoid breaking lines with \\ whenever possible. This should probably be written:

    RESOURCE_MAP['application/vnd.reviewboard.org.diff-validation'] = (

        DiffValidationResource)

david Oct. 6, 2014, 1:44 p.m.

Actually, when it's just a single line that's wrapping like this, we do use \.

asalahli Oct. 10, 2014, 10:52 p.m.

I copied this style from other similar lines in that file. I'm guessing I should drop this issue.

Change Summary:

Moved small issues mainly about coding style.

Commit:

eb61604ce95a7a9f25aeef9914c4528dd39f5cdc

ad488ba558657147c386ffb9498c96bac3ee3673

Diff:

Revision 3 (+51)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py

Change Summary:

Now passing parent diff and base dir to validate_diff() method.

Summary:

[WIP] `rbt post` command uses API's DiffValidation endpoint before upload

`rbt post` command uses API's DiffValidation endpoint before upload

Testing Done:

		Tested `rbt post` with:
		1. Empty diff (error 219)
		2. Diff from a different repository (error 207)
		3. Non-diff file as a diff (error 224)
		4. Valid diff (success)
	+
	+	using:
	+	1. Git
	+	2. Mercurial
	+
	+	I'm having trouble installing `subvertpy`, so I haven't tested `base_dir` yet.

Commit:

ad488ba558657147c386ffb9498c96bac3ee3673

f9c33e6d322602589191fd79aa4e6d076748f3f3

Diff:

Revision 4 (+58)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py

Testing Done:: Tested rbt post with:
1. Empty diff (error 219)
2. Diff from a different repository (error 207)
3. Non-diff file as a diff (error 224)
4. Valid diff (success)

using:
1. Git
2. Mercurial

~
I'm having trouble installing subvertpy, so I haven't tested base_dir yet.
~
Also tested base_dir with a subversion reporitory.

rbtools/api/resource.py (Diff revision 4)

The issue has been resolved. Show all issues

Can you make sure that everything here uses single quotes?

rbtools/commands/post.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you use single quotes for these strings?
```

Change Summary:


Replaced double quotes with singe quotes in some places
Removed dublicate code for repository and base_dir

Commit:

f9c33e6d322602589191fd79aa4e6d076748f3f3

a288b2c139d4a5fb00b93932aaca6f191fba27f3

Diff:

Revision 5 (+63 -9)

Show changes

	rbtools/api/resource.py
	rbtools/commands/post.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/post.py
    rbtools/api/resource.py

Could you add the group 'students' to the reviewers?

AS

asalahli Oct. 19, 2014, 11:20 a.m.
```
Done
```

Groups:: students

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-0.6.x (9b18656)