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

Added metrics mock-up #23

Open
wants to merge 1 commit into
base: master
from

Conversation

@sjmathew
Copy link

sjmathew commented Nov 18, 2018

#22

@@ -31,7 +31,7 @@ New contributors are always welcome!

Need to contact other collaborators in this project? Feel free to [join our slack group!](https://join.slack.com/t/githubdashboard/shared_invite/enQtNDcxMTM5OTMyNjExLWFmYTE1NTFiMzkyMzU0ZmRjMjI0YjI1OTVkMDk0MTUyZmJlMjM2NGUzODQ1YjZmZDVkMzkxYzgzYjM1MjI1ZGI)

## Liscense
## License

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

Unrelated changes should go in their own PRs. Do one thing per PR.

const octokit = require('@octokit/rest')()
const fs = require('fs');

var gitToken = "thisToken"

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

This needs to come from an environment variable or the like, you can't leave it like this. Also, you're not using it below, which you should.

type: 'public'
})

var targetProjectRoot = "C:/Users/SHAWN/data/"

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

This can't be hard coded, once again I'd suggest an environment variable. That way each dev, or the server, can use whatever they want. process.env gives you access to these.


downloadGitHubRepo(github, targetProjectRoot)

async function downloadGitHubRepo(github, targetDirectory) {

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

An async function needs to return a Promise. You should add a return on line 22 to achieve this.


const binaryExtensions = ['png', 'jpg', 'tiff', 'wav', 'mp3', 'doc', 'pdf']
var maxSize = 1000000;
function processGithubDirectory(owner, repo, ref, path, sourceRoot, targetRoot) {

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

Above you are using async/await style, and here using Promise.then() chaining. I'd suggest picking one or the other. async/await is probably best, so this code could be changed to:

try {
  const result = await octokit.repos.getContent({ "owner": owner, "repo": repo, "path": path, "ref": ref });
  // use result here...
catch (err) {
 // handle errors
}
octokit.repos.getContent({ "owner": owner, "repo": repo, "path": item.path, "ref": ref })
.then(result => {
var target = `${targetRoot + item.path}`
if (binaryExtensions.includes(item.path.slice(-3))) {

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

Prefer to use the path module and extname().

var target = `${targetRoot + item.path}`
if (binaryExtensions.includes(item.path.slice(-3))) {
fs.writeFile(target
, Buffer.from(result.data.content, 'base64'), function (err, data) { reportFile(item, target) })

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

Deal with err case in your callback.

, Buffer.from(result.data.content, 'base64'), function (err, data) { reportFile(item, target) })
}

else

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

Odd indenting here.

}

else
fs.writeFile(target

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

fs.writeFile(target, Buffer.from(result.data.content, 'base64'), function (err) {
  if (err) {
    return console.error(`Error writing content to disk: ${err.message}`);
  }
 reportFile(item, target);
});

function checkDirectorySync(directory) {
try {
fs.statSync(directory);

This comment has been minimized.

Copy link
@humphd

humphd Dec 14, 2018

I would advise against mixing sync with the rest of your async fs stuff above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.