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

Clarify comments + add field handler to multipart sample #609

Merged
merged 3 commits into from
May 24, 2018

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented May 3, 2018

Do not merge until the docs have been updated accordingly.

cc @marekbiskup

@ace-n ace-n requested a review from jmdobry May 3, 2018 02:34
@codecov
Copy link

codecov bot commented May 3, 2018

Codecov Report

Merging #609 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #609   +/-   ##
=======================================
  Coverage   48.52%   48.52%           
=======================================
  Files           1        1           
  Lines          68       68           
=======================================
  Hits           33       33           
  Misses         35       35

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bce81f4...09ece12. Read the comment docs.

@ace-n ace-n changed the title Clarify comments + add field handler to multipart sample Clarify comments + add field handler to multipart sample May 3, 2018
@ace-n ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 3, 2018
// This object will accumulate all the uploaded files, keyed by their name.
const uploads = {};
const tmpdir = os.tmpdir();

// This callback will be invoked for each file uploaded.
// This event will be triggered for each non-file field in the form.

Choose a reason for hiding this comment

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

I'm a bit afraid that users may confuse events described above, with event that trigger a function. For instance, users may think that there are many function executions here.

I think the previous form ("This callback will be invoked") was a bit better

Also, busboy docs say that events are emitted, not triggered.

How about something like this:

  • "This code is executed on..."
  • "This code handles busboy events emitted on..."
  • "This busboy events are emitted on..."

@@ -148,20 +148,31 @@ exports.uploadFile = (req, res) => {
if (req.method === 'POST') {
const busboy = new Busboy({ headers: req.headers });

// This object will accumulate all the fields, keyed by their name
const fields = {};

// This object will accumulate all the uploaded files, keyed by their name.
const uploads = {};
const tmpdir = os.tmpdir();

Choose a reason for hiding this comment

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

I would add an empty line before:
const tmpdir = os.tmpdir();

Otherwise it looks like the comment "This object will accumulate all the uploaded files, keyed by their name" is for the two lines.

You can group this "const tmpdir" together with "const busboy"

@ace-n ace-n removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label May 24, 2018
@ace-n ace-n merged commit 5007c6b into master May 24, 2018
@ace-n ace-n deleted the gcf-unify-multipart branch May 24, 2018 23:09
NimJay pushed a commit that referenced this pull request Nov 10, 2022
NimJay pushed a commit that referenced this pull request Nov 10, 2022
ace-n pushed a commit that referenced this pull request Nov 14, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 21, 2022
ace-n pushed a commit that referenced this pull request Nov 21, 2022
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