Summary: | webkit-patch does not prompt username if the default one is incorrect | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Victor Wang <victorw> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, commit-queue, dbates, eric | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Victor Wang
2010-02-01 11:10:24 PST
I'm not sure what the "default" username is. And is this for bugzilla or svn? Not sure if this bug is related, but I can confirm that for svn, unless you have initially checked out the repo. with a username or committed a change by hand (and overridden the username with the --username flag) then the default username is the login username. (In reply to comment #1) > I'm not sure what the "default" username is. > > And is this for bugzilla or svn? This is for svn. Agree with Daniel, sounds like the default username is from the login username. In this bug, I am more concerning about webkit-patch does not prompt "username" option if the one it tries is incorrect. I would like to be able to specify a username when I run the script or enter the right one if the one the script is using is incorrect. Created attachment 48209 [details]
Patch with unit tests
I am open to suggestions on better names for methods and variables as well as suggestions on the approach.
The code around raw_input should be in the user.py module. Maybe add a retry option to prompt() somehow? I think we have a similar idiom in bugzilla.py for the bugzilla password that should use the same code. Comment on attachment 48209 [details]
Patch with unit tests
Will look into factoring the code based on Adam's comment.
Created attachment 49015 [details]
Patch with unit tests
Comment on attachment 49015 [details]
Patch with unit tests
I would have made this generic:
307 def has_webkit_authorization_file(home_directory=os.getenv("HOME")):
not webkit-specific. Or at least have a generic version. Since it looks at the values from SVN itself, it seems no need to have _webkit_ in the name. It could use those values as default values to arguments passed to it.
I would have written this using List.extend:
373 svn_commit_args.append('--username')
374 svn_commit_args.append(username)
svn_commit_args.extend(["--username", username])
I think we're trying to standardize on " instead of ' anyway. You can ask Adam for more infos.
Making this static just makes it harder to mock/unit test:
6 @staticmethod
307 def has_webkit_authorization_file(home_directory=os.getenv("HOME")):
I suggest not making it static.
Otherwise I think this looks OK.
Clearly we don't have any unit testing of the actual commit, since the SVN.has_webkit_authorization_file call would actually be hitting disk for the real auth file in those cases!
r- for the above nits.
svn_server_realm = "<http://svn.webkit.org:80> Mac OS Forge" 261 svn_server_host = "svn.webkit.org" Should just be instance variables. They can have some class-level defaults if you like. But eventually we really need some sort of webkit_config.py module which holds all this webkit-specific stuff. Not that we ever plan to re-use webkit-patch for some other project, but it makes things read cleaner if we have all the webkit-specifc config in as few places as possible. Created attachment 49114 [details]
Patch with unit tests
Updated patch based on Eric's comments.
Comment on attachment 49114 [details]
Patch with unit tests
OK.
Comment on attachment 49114 [details] Patch with unit tests Clearing flags on attachment: 49114 Committed r55131: <http://trac.webkit.org/changeset/55131> All reviewed patches have been landed. Closing bug. |