-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attempt to look up label first before p4 changes -m1
#172
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments and unit tests
Unit tests are not optional for this type of change
if not result: | ||
return None # Revision spec had no submitted changes | ||
return result[0]['change'] | ||
try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check for changelist/label with re.match("^@?\d.$")
or .isdigit()
. Labels cannot be purely numeric.
Avoid using exceptions for flow control
This also potentially breaks some revision specifier formats, but I am not aware of anyone using anything other than labels and specific changelists. (e.g. @my-workspace-name
can sync to match your workspace)
return None # Revision spec had no submitted changes | ||
return result[0]['change'] | ||
try: | ||
label = self.perforce.run('label', ['-o', revision.replace('@','')]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
label = revision.lstrip('@')
labelinfo = self.perforce.run_label(['-o', label])
is more consistent with general codebase, accurate and DRY
Can also change result
below to changeinfo
for readability.
return result[0]['change'] | ||
try: | ||
label = self.perforce.run('label', ['-o', revision.replace('@','')]) | ||
return label[0]['Revision'].replace('@','') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if not labelinfo:
raise Exception('Could not detect head revision at label %s' % label)
Superseded by #181 |
Changes
Verification