Skip to content

Enhance select window - #407

Merged
just-be-dev merged 26 commits into
robotframework:masterfrom
divfor:Enhance-Select-Window
Jun 5, 2015
Merged

just-be-dev merged 26 commits into
robotframework:masterfrom
divfor:Enhance-Select-Window

Conversation

@divfor

@divfor divfor commented May 24, 2015

Copy link
Copy Markdown
Contributor

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

@divfor

divfor commented May 26, 2015

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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?

@just-be-dev

Copy link
Copy Markdown
Contributor

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

@divfor

divfor commented May 26, 2015

Copy link
Copy Markdown
Contributor Author

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.

@just-be-dev

Copy link
Copy Markdown
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.

@just-be-dev

Copy link
Copy Markdown
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

divfor commented May 29, 2015

Copy link
Copy Markdown
Contributor Author

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.

@just-be-dev

Copy link
Copy Markdown
Contributor

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

divfor commented Jun 5, 2015

Copy link
Copy Markdown
Contributor Author

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

@just-be-dev

Copy link
Copy Markdown
Contributor

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

@just-be-dev just-be-dev mentioned this pull request Jun 5, 2015
@just-be-dev

Copy link
Copy Markdown
Contributor

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

@divfor

divfor commented Jun 5, 2015

Copy link
Copy Markdown
Contributor Author

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

@just-be-dev

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
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

divfor commented Jun 5, 2015

Copy link
Copy Markdown
Contributor Author

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

just-be-dev added a commit that referenced this pull request Jun 5, 2015
@just-be-dev
just-be-dev 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