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

feat: Support HAR v1.3 proposal #5

Merged
merged 1 commit into from
Feb 14, 2019
Merged

Conversation

rbtcollins
Copy link
Contributor

I found debugging the failing parsing excruciating with the
untagged enum approach; I hope you'll consider the refactoring I've
done here, though if its a problem, now that it parses, I can switch
it back to untagged.

@mandrean mandrean changed the title Issue 3: Support the draft 1.3 HAR format feat: Support HAR v1.3 proposal Feb 14, 2019
Copy link
Owner

@mandrean mandrean left a comment

Choose a reason for hiding this comment

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

Looks good!

I updated the PR title to follow semantic-rs as I'm experimenting with it in this repo. I kinda like the auto-generated changelogs, auto-semver bumping & publishing to crates.io etc.

Could you maybe move up headersCompression so it's right under headersSize? Like in the 1.3 proposal. Same with encoding. OCD 🤷‍♀️

Otherwise awesome work 🙌

@mandrean mandrean mentioned this pull request Feb 14, 2019
I found debugging the failing parsing excruciating with the
untagged enum approach; I hope you'll consider the refactoring I've
done here, though if its a problem, now that it parses, I can switch
it back to untagged.
@rbtcollins
Copy link
Contributor Author

Changes done

Copy link
Owner

@mandrean mandrean left a comment

Choose a reason for hiding this comment

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

👍

@mandrean mandrean merged commit 3d7c529 into mandrean:master Feb 14, 2019
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.

2 participants