Review Request #1508: ClearCase UnboundLocalError instead communicate with informations

Information
Owner:	jan.koprowski
Repository:	RBTools
Branch:	master
Bugs:	1599
Depends On:
Groups:	reviewboard
People:

DJ

rbtools/postreview.py (Diff revision 2)

Use double quotes instead of escapes if needed.

Grammar:

File %r does not exist or unknown viewtype (%s)

jan.koprowski April 9, 2010, 3:55 p.m.

"unknown" viewtype is not the same as "viewtype is not adequat". There is extend_path in ClearCase like filename.ext@@/main/branch/VERSION and some of them work only in dynamic view. So You can search file /my/development/path/filename.ext@@/main/branch/VERSION in "snapshot" view and You get error but not because viewtype is unknown but is not adequat (should be dynamic).

This is mistake which I made many times when I starting with post-review. I just forgot to change viewtype in post-review.
From the other hand maybe this is good moment to add parameter "--view-type"?

Change Summary:

New diff with change allowing pass ClearCase --view-type in parameters

Diff:

Revision 3 (+20 -8)

Show changes

rbtools/postreview.py

Change Summary:

Remove nasty white spaces.

Diff:

Revision 4 (+19 -8)

Show changes

rbtools/postreview.py

Change Summary:

Change grammar following the reviewer advices.

Diff:

Revision 5 (+19 -8)

Show changes

rbtools/postreview.py

DJ

The word should be 'adequate' not 'adequat'. I suggested 'unknown' because I'm not quite sure what you were trying to suggest. I have some knowledge of CC and have tested RB with CC, but I don't normally use, so I'm not quite sure what state everything is in. Last I checked, I could only get post-review to work with dynamic views. Is your error message meant to indicate this? I imagine we could detect the type of view with a cleartool command, instead of specifying it as an option.

jan.koprowski April 11, 2010, 9:39 p.m.

1) Specifying viewtype using cleartool is good idea, I will try do this.
2) Post-review works only with dynamic view now. Question is there can work with snapshots? Ever? For my knowledge (I'm not CC expert just maybe a little bit geeky user) paths like @@/main etc works only in dynamic view. Without that support in snapshots we can't get appropriate informations because @@ path wasn't work (I may be in mistake). Before I add --view-type "snapshot" was hardocoded. When "snapshot" was hardcoded and I use post-review in dynamic view I just get error because none of conditions was true and variable "fn" wasn't initialized. So I decide add else clause which return error which mean: You use post-review with --view-type = dynamic in snapshot view or you use post-review with --view-type = snapshot in dynamic view. This is the goal of this else clause.

DJ

djs April 12, 2010, 2:21 a.m.

That is my understanding as well. The extended path notation is provided by the special filesystem which only works if you're using that filesystem. For snapshot views, we'd need to get the file data from elsewhere. I don't have any idea how (other than editing the config spec), but I imagine there is probably a way.

What confused me is what the original author of the clearcase support was thinking, since the code implied to me that it does support snapshot views, somehow, especially given 'snapshot' is the default type of view that was hardcoded...

jan.koprowski April 12, 2010, 5:10 p.m.

So. Summaries.
1) Change hard coded information "snapshot" to "dynamic" is a good idea, because now hardcoded just make post-review useless as default for ClearCase.
2) For now we know dynamic ClearCase way to get needed informations about files but there is a loooong way to get post-review works with snapshot views as good as dynamic view work, and we didn't know how to do that now.
3) This will be good to check viewtype using cleartool or even back die("ReviewBoard post-review script does not work with snapshot views in this version. Please use dynamic view.") when was run in snapshot.
4) For now parameter --view-type is stupid thing (because one option doesn't work) so meybe hardcoded value even will be better but this parameter is very prospective for now (until auto recognized isn't work). You decide what should stay.

I think I don't have enough time for this issues in my work (else much more important waiting). I try back to this in the future. For now I will stay this like this. In this or next week I try update documentation about CC in git.

Change Summary:

Fix quotes and fatal error with stderr.write() mistake.
Replay

Diff:

Revision 6 (+19 -8)

Show changes

rbtools/postreview.py

DJ

rbtools/postreview.py (Diff revision 6)
```
Almost... still a typo here. Should be 'adequate'
```
1. jan.koprowski April 19, 2010, 12:08 a.m.
  Typo fixed :)

Change Summary:

Typo fixed.

Diff:

Revision 7 (+19 -8)

Show changes

rbtools/postreview.py

rbtools/postreview.py (Diff revision 7)
```
Can you rename this to --clearcase-view-type?
```
1. jan.koprowski June 2, 2010, 1:02 a.m.
  renamed

rbtools/postreview.py (Diff revision 7)

The variable should be named "clearcase_view_type".

Would it make sense to default this parameter to "snapshot" for existing users?

jan.koprowski June 2, 2010, 1:02 a.m.
```
renamed
```

rbtools/postreview.py (Diff revision 7)

"The view type used when generating the diff (ClearCase Only)"

jan.koprowski June 2, 2010, 1:02 a.m.
```
changed
```

rbtools/postreview.py (Diff revision 7)

Multi-line conditionals should use (...) instead of \

jan.koprowski June 2, 2010, 1:02 a.m.

One condition behind that use \ not (...)  so I use \.  I change to one line.

rbtools/postreview.py (Diff revision 7)

This makes it sound like the parameter doesn't exist. The error message should better describe that the view type isn't set and needs to be. Something like:

"The --clearcase-view-type parameter must be specified for ClearCase repositories."

jan.koprowski June 2, 2010, 1:01 a.m.
```
Changed.
```

Diff:

Revision 8 (+18 -8)

Show changes

rbtools/postreview.py

rbtools/postreview.py (Diff revision 8)

This is going to break some existing setups, I'd imagine. I'd be much happier if this could work like repository information for other SCMs. Make it a customizable setting in .reviewboardrc for the repository, so that the user will only have to pass it if it's not found there.

jan.koprowski July 2, 2010, 2:24 a.m.

I have no known about any Clear Case ability to keep additional informations or any meta-data. I will ask in my company but for all I know Clear Case can't do that.

Change Summary:

Only dynamic view is able to show any version existing in repository. Snapshot view is unable to be used as information about other versions then checked out (this is like a photo of the repository take in selected moment. You can't see what was happen before or after). Only way to implement snapshot views in ClearCase is to add something like --compare-directories="/path/to/directory1:/path/to/directory2". I show similar implementation in one compering tool which get two distinct snapshot views to compare them.

Diff:

Revision 9 (+4 -1)

Show changes

rbtools/postreview.py

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

ClearCase UnboundLocalError instead communicate with informations

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Diff:

Change Summary:

Diff: