Skip to content

Conversation

@divfor
Copy link
Contributor

@divfor divfor commented May 24, 2015

this is a re-submit for PR #395 - rebase to latest rotmac/master to resolve conflict

@divfor
Copy link
Contributor Author

divfor commented May 26, 2015

Hey @zephraph , I have done this PR and passed all my testing. Please review for merge.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I might be missing something. If they provide a locator, couldn't this possible return the wrong window?

@zephraph
Copy link
Contributor

@emanlove, @pekkaklarck, @ombre42 Can any of you assist in this review?

@divfor
Copy link
Contributor Author

divfor commented May 26, 2015

actually the 'return self._current_browser().get_current_window_handle()' is making a result and store it only. after try block the finally block will be executed, at last the function ends and the stored return value is pop up to caller.
I test this and it works well.

@zephraph
Copy link
Contributor

Overall it looks good to me, but I want to give it another day or two to let someone else look at it before I merge.

@zephraph
Copy link
Contributor

Hey @divfor, I'm sorry this is taking so long. Being as we haven't heard from anyone else, I'll take some extra time tonight and play around with this until I'm comfortable that I understand exactly everything you're doing. If I can't break it then I'll merge it tomorrow.

@divfor
Copy link
Contributor Author

divfor commented May 29, 2015

Thank you, zephraph.
One more notes: besides my project using it, I also sent this enhancement to some other companies' guys for 2 weeks and did not receive negative feedback.

@zephraph
Copy link
Contributor

zephraph commented Jun 4, 2015

Hey @divfor, I'm planning on releasing 1.7 today.

Do you want this in that release, or is it okay to wait for 1.7.1? If you want it in today, please merge with master as soon as you're able.

Thanks!

@divfor
Copy link
Contributor Author

divfor commented Jun 5, 2015

I tried but can not fix the conflicts in CHANGES.rst...
could you help?

@zephraph
Copy link
Contributor

zephraph commented Jun 5, 2015

So we're you able to get it? I can do a PR on your fork if you need.

@zephraph zephraph mentioned this pull request Jun 5, 2015
@zephraph
Copy link
Contributor

zephraph commented Jun 5, 2015

Tell me when you're done making changes and I'll open a PR on your branch to do the merge.

@divfor
Copy link
Contributor Author

divfor commented Jun 5, 2015

I still cannot fix the conflicts. please start from the latest 'Update CHANGES.rst' -- aa50ade.

@zephraph
Copy link
Contributor

zephraph commented Jun 5, 2015

Hopefully this builds successfully! I'm really ready to push out this release...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be better to only switch to the window when you know that it meets the right criteria? If it's unable to locate the handle you want to switch to, I would think the expected behavior would be to stay on the current handle.

@divfor
Copy link
Contributor Author

divfor commented Jun 5, 2015

Yes, we can add it like what _select_matching() does.
I ignored it because a VauleError exception must be raised to fail this keyword if no match window found

zephraph added a commit that referenced this pull request Jun 5, 2015
@zephraph zephraph merged commit 783580b into robotframework:master Jun 5, 2015
@divfor divfor deleted the Enhance-Select-Window branch June 7, 2015 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants