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

Fetch: enable cookies and storing error messages #141

Closed
wants to merge 8 commits into from

Conversation

mhuang74
Copy link
Contributor

@mhuang74 mhuang74 commented Jan 5, 2022

Improve Fetch (#77) with cookies and storing error messages.

  • Add implementation, tests, and docs for --store-error flag.
  • Support --cookies flag.
  • Remove --jobs param since single-thread may be fast enough for most usecases.
  • Update examples in Fetch.md

@mhuang74 mhuang74 changed the title Fetch: enable cookies and storing error messages Fetch: enable cookies and storing error messages (#77) Jan 5, 2022
@mhuang74 mhuang74 changed the title Fetch: enable cookies and storing error messages (#77) Fetch: enable cookies and storing error messages Jan 5, 2022
@jqnatividad jqnatividad self-requested a review January 5, 2022 16:57
Copy link
Collaborator

@jqnatividad jqnatividad 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 this @mhuang74 ,

The fetch_ratelimit test is failing for "Hefty Smurf" - the 5th element with --rate-limit set to 4.

Perhaps the result is inadvertently dropped when you reach the limit? But why only in Windows?

At first, I thought it might be a Github Actions runner issue, but I even tried running your PR locally on my Windows laptop and I got the same error.

Cargo.toml Show resolved Hide resolved
src/cmd/fetch.rs Outdated Show resolved Hide resolved
@mhuang74
Copy link
Contributor Author

mhuang74 commented Jan 6, 2022

Thanks @jqnatividad. I've increased burst size to 7, while keeping everything else the same, to see if error goes away.

Other issues corrected.

Copy link
Collaborator

@jqnatividad jqnatividad 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!

@jqnatividad
Copy link
Collaborator

jqnatividad commented Jan 6, 2022

Closing as GH is giving false positive unmerged commits message, but it got merged just the same...
Can you confirm @mhuang74?

@jqnatividad jqnatividad closed this Jan 6, 2022
@mhuang74
Copy link
Contributor Author

mhuang74 commented Jan 7, 2022

Closing as GH is giving false positive unmerged commits message, but it got merged just the same... Can you confirm @mhuang74?

Confirmed that all 8 commits have been merged into master. Thanks @jqnatividad!

Probably user error in VSCode that this commit got pushed to upstream directly. Oops.

e1a8a8b

UPDATE: nevermind, wasn't reading the unsquashed commit history correctly...all good.

mhuang74 pushed a commit to mhuang74/qsv that referenced this pull request Jan 10, 2022
Fetch: enable cookies and storing error messages
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