Skip to content

Use fs.access instead of fs.existsSync - #350

Merged
thefourtheye merged 3 commits into
masterfrom
use/fs.access
Nov 11, 2015
Merged

Use fs.access instead of fs.existsSync#350
thefourtheye merged 3 commits into
masterfrom
use/fs.access

Conversation

@lpinca

Copy link
Copy Markdown
Member

The reason behind this change is that fs.exists and its sync version are deprecated.

Comment threadscripts/tasks/download.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since you are refactoring, you can directly create evRepo without those intermediate requires.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sure.

@thefourtheye

Copy link
Copy Markdown
Contributor

Change LGTM. Do you know why CI is not run for this PR?

@lpinca

Copy link
Copy Markdown
MemberAuthor

No clue.

@lpinca

Copy link
Copy Markdown
MemberAuthor

@thefourtheye please check again.

@phillipj

Copy link
Copy Markdown
Member

LGTM. Some error handling on the HTTPS request and file writing would be nice, but that could be another PR.

Comment threadscripts/tasks/download.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be better if we notified the user once the downloading is complete?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Any idea on what kind of message? Something like this?

console.log(`Weekly Update ${filePath} downloaded.`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We already log the filePath. So wouldn't Download completed be better?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

It would help to understand what file the message refers to, otherwise with multiple files, it can be confusing, but yes a simple "Download completed" can work too.

@thefourtheye

Copy link
Copy Markdown
Contributor

@phillipj do you know why CI is not run for this PR?

Comment threadscripts/tasks/download.js Outdated

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@thefourtheye If you feel strong about using only "Download completed.", I'll change this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This actually should have been

response.on('end',()=>console.log(`Weekly Update ${filePath} downloaded.`))

@phillipj

Copy link
Copy Markdown
Member

@thefourtheye no idea why it doesn't show inline in the PR.. These commits are visible in the Travis/new.nodejs.org build history though, might be a github hickup?

@lpinca

Copy link
Copy Markdown
MemberAuthor

PTAL.

@thefourtheye

Copy link
Copy Markdown
Contributor

Still looks fine to me. But I would be confident if CI ran.

@phillipj

Copy link
Copy Markdown
Member

thefourtheye added a commit that referenced this pull request Nov 11, 2015
Use `fs.access` instead of `fs.existsSync`
@thefourtheye
thefourtheye merged commit 87912b7 into masterNov 11, 2015
@thefourtheye
thefourtheye deleted the use/fs.access branch November 11, 2015 10:43
@thefourtheye

Copy link
Copy Markdown
Contributor

Cool. Merged now. 👍 Thanks @phillipj :-)

Sign up for freeto 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

@lpinca@thefourtheye@phillipj