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

Espresso SDK #138

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

Espresso SDK #138

wants to merge 30 commits into from

Conversation

kb-kerem
Copy link
Contributor

@kb-kerem kb-kerem commented Sep 19, 2024

Open points:

  • Environment variable handling needs to be adapted to Android best practices Handled it with buildConfig and localProperties
  • Builder classes where appropriate (VisualClient, VisualCheckOptions, BuildAttributes etc.) Provided
  • Use default Android http client instead of Apache's Switched to using apollo graphql client

Copy link
Contributor

@konraddysput konraddysput left a comment

Choose a reason for hiding this comment

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

Can we please add comments to the public API?

@omacranger
Copy link
Member

A lot of the code here looks similar-ish to the Java client. Can none of that be imported and used here as a dependency somehow?

@kb-kerem
Copy link
Contributor Author

kb-kerem commented Sep 24, 2024

A lot of the code here looks similar-ish to the Java client. Can none of that be imported and used here as a dependency somehow?

@omacranger That would be super convenient however it brings also some problems that complicates the implementation. Importing Java client brings transitive dependencies that are not compatible with Android environment, considering the target Android API (apache httpclient, graphql-java etc.). Those dependencies need to be excluded explicitly and need to be maintained for the future versions of java client if new ones show up. Also, since we are using the same package and class names for certain classes that need reimplementation (mainly VisualAPI and GraphQLClient), class/method loading problems arise in runtime. Instead of trying to fix those problems, I'd rather keep the implementations fully separate.

NVM, I've managed to get it running by changing the main package name from com.saucelabs.visual to com.saucelabs.visual.espresso. There's a clear separation of packages now so no more class/method loading problems arise.

@kb-kerem kb-kerem changed the title Espresso SDK (poc) Espresso SDK Oct 2, 2024
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