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

Add resumable to the uploadOptions #43

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

imuchene
Copy link

@imuchene imuchene commented Mar 7, 2021

  • Added resumable as to the uploadOptions with a default value of true.

@lgtm-com
Copy link

lgtm-com bot commented Mar 7, 2021

This pull request introduces 1 alert when merging 9730b52 into d439945 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@DrPaulBrewer
Copy link
Owner

Thank you for sending this patch. It is going to be a while yet before I can verify the issue...

The patch was reviewed by code quality automation and generated an alert for

'Useless assignment to local variable' for line 42.

Essentially, the new line 42 is not setting resumable in the passed options object. I played around with some similar
code in nodejs REPL and confirm it only sets in the local scope and not in the options object.

This could be fixed elsewhere. The original work of the validate function was to throw an error on invalid input, not parse or correct the input.

However, this also means that when options.resumable is undefined it is not being reset to a default of true.

For these reasons I cannot approve the patch as it exists.

Thank you again for volunteering your time to look at this and of course any comments or additional research is welcome.

@lgtm-com
Copy link

lgtm-com bot commented Jan 6, 2022

This pull request introduces 4 alerts when merging 791a907 into d439945 - view on LGTM.com

new alerts:

  • 2 for Unneeded defensive code
  • 1 for Useless assignment to local variable
  • 1 for Assignment to constant

@lgtm-com
Copy link

lgtm-com bot commented Jan 7, 2022

This pull request introduces 1 alert when merging 9952321 into d439945 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

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