Skip to content
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

Created ViewActionExecutor to reuse performAction() code #194

Merged
merged 7 commits into from
Jul 2, 2018

Conversation

jamesbluecrow
Copy link
Contributor

No description provided.

@jamesbluecrow
Copy link
Contributor Author

@Sloy @rocboronat I've created the class ViewActionExecutor just so we can reuse code, thanks to it I've removed the duplication that myself adde when supporting the keyboard actions.

Let me know what you think (and help me out with the name, I'm not sure what to call it, so far ViewActionExecutor was the best I could think of.

@Sloy
Copy link
Member

Sloy commented Dec 15, 2017

Haha today I was just doing the same refactor for the similar ugliness that we have in Assertions, but with a different approach. Let me finish it during the weekend and see how they fit together.

@jamesbluecrow
Copy link
Contributor Author

:) Awesome. I just was trying to add the swipe interactions and realized that I would need again this functionality, so I extracted it.
Do you want me to close this? I just did moved the code from one place to another.
Ping me when you finish your refactor so I can take a look too.

@Sloy
Copy link
Member

Sloy commented Dec 16, 2017

For now I would say just copy the code again for your functionality. We know we'll fix it soon enough.
I have something almost ready, but it might raise discussion. Will ping you ;)

I'd say keep this PR open for now and we'll see later.

@Sloy Sloy added the blocked label Dec 16, 2017
@SmasSive
Copy link
Member

Let's wait for @Sloy proposal but looks great to me since it concentrates the ugliness in just one class! Easier to beautify it later! 💄

@Sloy
Copy link
Member

Sloy commented Dec 18, 2017

@jamesbluecrow check out #198

Sloy added 3 commits June 28, 2018 12:06
I changed it a bit to make it similar to AssertAny, the current equivalent common function for assertions
I skipped the one with very custom implementations and don't fit with the common pattern
@Sloy
Copy link
Member

Sloy commented Jun 28, 2018

Hi! I took the liberty of reviving the PR. I updated your code @jamesbluecrow to match the decisions made in #198. I kept the name performAction, although it sounds very generic and similar to Espresso's perform. I can't think of a name that reflects the real behavior of the method, there's too much magic in there.

After confirming the name and adding a bit of javadoc, I think we can mark this code as ready 😄

@Sloy Sloy requested review from rocboronat, alorma and SmasSive and removed request for rocboronat June 28, 2018 12:54
Copy link
Contributor Author

@jamesbluecrow jamesbluecrow left a comment

Choose a reason for hiding this comment

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

LGTM
Great work @Sloy 👌

@Sloy
Copy link
Member

Sloy commented Jul 2, 2018

No more news is good news. Let's merge it! Thanks for the contribution @jamesbluecrow and sorry for the delay 🎁

@Sloy Sloy merged commit 1775197 into AdevintaSpain:master Jul 2, 2018
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.

3 participants