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

Export To Har #880

Merged
merged 20 commits into from Sep 29, 2022
Merged

Conversation

BluestormDNA
Copy link
Contributor

@BluestormDNA BluestormDNA commented Sep 10, 2022

Edited the export function that writes transaction data to a file and
returns a URI to access that file to also support .HAR format as a parameter.

πŸ“· Screenshots

Internal Changes. The sample app layout has been updated to reflect the functionality: "Export to Har" button.

Screenshot_1663334392
Screenshot_1663334610

πŸ“„ Context

Closes #871
It adds support for writeTransactions to also export on .HAR format

πŸ“ Changes

Pending.

πŸ“Ž Related PR

None

🚫 Breaking

None

πŸ› οΈ How to test

It can be tested from the UI just adding the ExportFormat

⏱️ Next steps

No.

@cortinico cortinico marked this pull request as draft September 10, 2022 15:43
@cortinico
Copy link
Member

Converting this to Draft. Feel free to ping me when you're done and need a review

@BluestormDNA
Copy link
Contributor Author

BluestormDNA commented Sep 10, 2022

Go ahead @cortinico
As I commented on #871:
Please be aware that the @SuppressLint("VisibleForTests") is there because HarUtils only exposes a suspend fun and i needed the underlying blocking method.
I'll await comments.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Thanks for sneding this over.

You're missing several critical changes:

  1. Please fill in the pull request template.
  2. Update the library-no-op variant of the library as well.
  3. Update the CHANGELOG with this change
  4. Add tests for this change.

@BluestormDNA
Copy link
Contributor Author

About tests: the writeTransactionsoriginal PR added a button to the Sample App to export.
Should i add another button below? or maybe put a radio button to select?

@cortinico cortinico marked this pull request as ready for review September 10, 2022 16:45
@cortinico
Copy link
Member

Should i add another button below? or maybe put a radio button to select?

Either two button side by side or a radio button work πŸ‘

@vbuberen
Copy link
Collaborator

Nothing to see, internal changes.

Seems like after adding a button there is now something to see. Would be good to have some screenshots.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Awesome stuff πŸ‘ Thanks for taking a stance at this

@cortinico
Copy link
Member

@vbuberen can we merge this?

@vbuberen
Copy link
Collaborator

Sure, looks good to me.

@vbuberen vbuberen merged commit 6fa5be2 into ChuckerTeam:develop Sep 29, 2022
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.

Add export function to API (.har file)
3 participants