Skip to content

Add resumable to the uploadOptions#43

Open
imuchene wants to merge 5 commits into
DrPaulBrewer:masterfrom
imuchene:master
Open

Add resumable to the uploadOptions#43
imuchene wants to merge 5 commits into
DrPaulBrewer:masterfrom
imuchene:master

Conversation

@imuchene

@imuchene imuchene commented Mar 7, 2021

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

@lgtm-com

lgtm-com Bot commented Mar 7, 2021

Copy link
Copy Markdown

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
Copy Markdown
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

lgtm-com Bot commented Jan 6, 2022

Copy link
Copy Markdown

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

lgtm-com Bot commented Jan 7, 2022

Copy link
Copy Markdown

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