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

refactor: removed async module usage #1

Closed
wants to merge 3 commits into from

Conversation

vijay-qlogic
Copy link
Owner

Fixes #2883 (it's a good idea to open an issue first for discussion)

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

const fs = require('fs');
const path = require('path');

const {promisify} = require('util');

Choose a reason for hiding this comment

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

line 24. the link is not working, and please add a full stop (period) for all the comments with complete sentences as some of them have full stops (line 33), and others don't.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done!
comments from line-21 to line-23 is single sentence, so not added full stop (period) there, elsewhere it's added.
link update to https://cloud.google.com/docs/authentication/getting-started, this page speaks as per the comments.

bb6ccac

@@ -89,7 +88,7 @@ Index.prototype.lookup = function(words, callback) {
self.tokenClient.smembers(word, cb);
};
});
async.parallel(tasks, callback);
Promise.all(tasks).then(callback);

Choose a reason for hiding this comment

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

Same comment here, I think you can just return Promise.all(taskss);
and line 83 make into
Index.prototype.lookup = async words => {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done! bb6ccac

@@ -78,7 +77,7 @@ Index.prototype.add = function(filename, document, callback) {
self.tokenClient.set(filename, document, cb);
});

async.parallel(tasks, callback);
Promise.all(tasks).then(callback);

Choose a reason for hiding this comment

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

I think you can just return Promise.all(tasks); here
and on line 60 make it into an async function
Index.prototype.add = async (filename, document) => {

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done !
bb6ccac

@AVaksman
Copy link

for getTextFromFiles() function:
refactor to

async function getTextFromFiles(index, inputFiles) {
  // Make a call to the Vision API to detect text
  const requests = inputFiles.map(filename => {
    return {
      image: {content: fs.readFileSync(filename).toString('base64')},
      features: [{type: 'TEXT_DETECTION'}],
    };
  });
  const [results] = await client.batchAnnotateImages({requests: requests});
  const detections = results[0].responses;
  const textResponse = {};
  const tasks = inputFiles.map((filename, i) => {
    const response = detections[i];
    if (response.error) {
      console.log('API Error for ' + filename, response.error);
      return;
    } else if (Array.isArray(response)) {
      textResponse[filename] = 1;
    } else {
      textResponse[filename] = 0;
    }
    return cb => extractDescriptions(filename, index, response, cb);
  });
  return Promise.all(tasks);
}

@AVaksman
Copy link

refactor main function to

// Run the example
async function main(inputDir) {
  const index = new Index();
  // Scan the specified directory for files
  const files = fs.readdirSync(inputDir);
  // Separate directories from files
  const allImageFiles = files.map(file => {
    const filename = path.join(inputDir, file);
    if (!fs.statSync(filename).isDirectory()) {
      return filename;
    }
  });
  // Figure out which files have already been processed
  const tasks = allImageFiles
    .filter(filename => filename)
    .map(filename => {
      return cb => {
        index.documentIsProcessed(filename, function(err, processed) {
          if (err) {
            return cb(err);
          }
          if (!processed) {
            // Forward this filename on for further processing
            return cb(null, filename);
          }
          cb();
        });
      };
    });
  // Analyze any remaining unprocessed files
  const imageFilesToProcess = await Promise.all(tasks).filter(
    filename => filename
  );
  if (imageFilesToProcess.length) {
    return getTextFromFiles(index, imageFilesToProcess);
  }
  console.log('All files processed!');
  return Promise.resolve();
}

const fs = require('fs');
const path = require('path');

const {promisify} = require('util');

Choose a reason for hiding this comment

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

after proposed refactoring promisify is not needed any more, please remove it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done!

@AVaksman
Copy link

This task was completed by JustinBeckwith as part of a bigger PR.
googleapis#297

@vijay-qlogic
Copy link
Owner Author

Alright! Closing this PR then.

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