Skip to content

Conversation

@Jonahss
Copy link
Member

@Jonahss Jonahss commented Aug 19, 2014

And removed complex-find while at it.

Submitting to @bootstraponline for review.

I'm working now on changing the getPageSource code to reuse the same code here.

Copy link
Member

Choose a reason for hiding this comment

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

authorship is tracked via git

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops yeah. Those get added by my ide by default.

@jlipps
Copy link
Member

jlipps commented Aug 19, 2014

LGTM. Pretty sure @bootstraponline will ask you to remove the "created by jonahss" comments though :-)

@bootstraponline
Copy link
Member

I'd like to do a better review on this once I'm back from the roadshow on Friday.

Choose a reason for hiding this comment

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

Do you know why?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because android UiAutomator library is notoriously flaky and error prone :(

@bootstraponline
Copy link
Member

ok. I think this is fine to merge. jlipps wants to release 1.2.2. I'll do a proper review later and that doesn't block the pr.

@bootstraponline
Copy link
Member

also remember to squash to one commit

Copy link
Member

Choose a reason for hiding this comment

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

why do we need continue?

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably had code following the try/catch, which I later deleted.
It's nice to have the continue in there, so if a developer casually added some logic to that iteration, they wouldn't accidentally forget to add it.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should add a continue unless it's necessary. It's a for loop so we're going to continue either way.

@Jonahss
Copy link
Member Author

Jonahss commented Aug 20, 2014

looks like travis caught an error. I removed the npm dependency on xpath without realizing that iOS needs it.
The library is kind of buggy though :(

I'll squash these commits. but does anything else need to be changed for merge?

We can create a separate issue for adding stack-trace to errors.

@jlipps
Copy link
Member

jlipps commented Aug 20, 2014

Looks good to me! Do all the appium xpath tests still pass with this?

…s buggy and adheres to the full xpath spec. Finding by XPath now operates on the uncompressed xml hierarchy. Fixes appium#3307
@Jonahss
Copy link
Member Author

Jonahss commented Aug 20, 2014

squashed.

Yup, appium xpath tests pass.

@bootstraponline
Copy link
Member

I'll squash these commits. but does anything else need to be changed for merge?

I think it's fine. I don't think we need the continue, otherwise 👍 from me.

@jlipps
Copy link
Member

jlipps commented Aug 21, 2014

agreed about the interesting continue. let's axe that and merge this baby.

@Jonahss
Copy link
Member Author

Jonahss commented Aug 21, 2014

removed that continue statement.
added a comment about our giant Exception catchall

Jonahss added a commit that referenced this pull request Aug 21, 2014
Moved xpath into boostrap java
@Jonahss Jonahss merged commit fd34730 into appium:master Aug 21, 2014
@bootstraponline
Copy link
Member

@Jonahss hey, you said you would squash.

@bootstraponline
Copy link
Member

I guess it's about 3 different issues so the different commits are fine.

@bootstraponline
Copy link
Member

I had time to test this today and it all seems to be working. It'd be nice to have the Java bootstrap tests run automatically if that's not already setup.

@Jonahss
Copy link
Member Author

Jonahss commented Aug 25, 2014

Agreed. I'll create an issue.

On Sun, Aug 24, 2014 at 7:42 PM, bootstraponline [email protected]
wrote:

I had time to test this today and it all seems to be working. It'd be nice
to have the Java bootstrap tests run automatically if that's not already
setup.


Reply to this email directly or view it on GitHub
#3428 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants