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

implemented har feature #7970

Merged
merged 56 commits into from
Jul 30, 2024
Merged

Conversation

hrajwade96
Copy link
Contributor

@hrajwade96 hrajwade96 commented Jun 23, 2024

  • Export har feature added.
  • includes serialisation and de-serialisation (from and to har).
  • test cases added.

This PR addresses below issue -
#3806

Please add a note to packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md if your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or there is a reason for not adding tests.

build.yaml badge

If you need help, consider asking for help on Discord.

@hrajwade96 hrajwade96 requested review from kenzieschmoll, bkonyi and a team as code owners June 23, 2024 07:58
@hrajwade96
Copy link
Contributor Author

@kenzieschmoll @exaby73
When I disconnect the app loader keeps showing #7968

Copy link
Contributor

@bkonyi bkonyi 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 the contribution! I've got mostly minor comments so far.

@hrajwade96 hrajwade96 requested a review from bkonyi June 24, 2024 17:32
Copy link
Contributor

@bkonyi bkonyi 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 addressing the comments!

};
}).toList();

// Assemble the final HAR object
Copy link
Contributor

Choose a reason for hiding this comment

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

Ubernit: indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry didn't get you

static const onContentLoad = -1;
static const onLoad = -1;
static const httpVersion = 'HTTP/1.1';
static const responseHttpVersion = 'http/2.0';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this field doesn't seem to be available in the 'DartIOHttpRequestData'

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put some placeholder like "Unknown" instead? Maybe @brianquinlan knows if this information is available somewhere.

@hrajwade96 hrajwade96 requested a review from bkonyi June 27, 2024 08:14
@kenzieschmoll
Copy link
Member

When I disconnect the app loader keeps showing #7968

This issue should be resolved if you merge with the master branch. Fix was here: #7992

@hrajwade96
Copy link
Contributor Author

hrajwade96 commented Jul 6, 2024

@kenzieschmoll as I observed the offline functionality works in these ways :

  1. When the app gets disconnected, view offline data.
  2. Before connecting to the app, few screens support importing a file.
  3. While the app is connected user can still import a saved file, it disconnects the app and in goes in offline mode where dev can view the data.

I have implemented the 1st one (changes yet to be pushed as of now).
Can you clarify which of these is req for network screen?
this is my demo pr for 1st point from my forked repo - hrajwade96#1
let me know your inputs

@hrajwade96
Copy link
Contributor Author

@bkonyi can you re-check the comments and mark them as resolved if they are addressed

@bkonyi
Copy link
Contributor

bkonyi commented Jul 8, 2024

@bkonyi can you re-check the comments and mark them as resolved if they are addressed

Sorry for the delay, I'll take a look.

@bkonyi
Copy link
Contributor

bkonyi commented Jul 8, 2024

All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution!

@hrajwade96
Copy link
Contributor Author

All of my comments are basically addressed and this LGTM overall. I'll let @kenzieschmoll give final say on the PR. Thanks for your contribution!

Looking forward to contributing more in future, after this one is done!

Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

I apologize, I had a pending review that I never hit submit on. Here are my recommendations.

Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

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

LGTM once all tests pass. Thanks for the feature!

@hrajwade96
Copy link
Contributor Author

LGTM once all tests pass. Thanks for the feature!

Thank you for the review! I'll ensure all tests pass. Appreciate the feedback and support!

@hrajwade96
Copy link
Contributor Author

LGTM once all tests pass. Thanks for the feature!

@kenzieschmoll All the tests have passed can this be merged now? Or Anything else remaining?

@kenzieschmoll kenzieschmoll merged commit fae408a into flutter:master Jul 30, 2024
23 checks passed
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.

3 participants