-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add additional API features #10
base: master
Are you sure you want to change the base?
Add additional API features #10
Conversation
This is a large commit since I started working on this project before I started using GitHub, and I didn't want to leave the README.md file behind.
This reverts commit b41c3ff.
index.js
Outdated
var that = this | ||
var returning = false | ||
|
||
if (!this.siteurl) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A convention in using curly-braces could be considered here, including the curly-braces will make this easier to read for others following along the code.
file.on('finish', function() {file.close((dwnldFile))}) | ||
that.client.get(that.siteurl.href + files[i].name.replace(/^\/?/, '/'), function(data) {return data.pipe(file)}) | ||
} | ||
// else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its good practice to include commented code, its better to refer the commented block for discussion. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured out what that commented block was intended to do. It was the start of some code that allows files to be downloaded to a variable to return without saving them to the system. The use case would be if the file is needed for immediate use, but not for later. If you think that it would be good for this library to have that feature, I can finish adding it. Otherwise, it can just be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any comment on this feature?
function list(dir) { | ||
activePaths.push(dir) | ||
fs.readdir(dir, parseFiles) | ||
function parseFiles(err, dirContents) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be cleaner and easier for others to understand if it had better logical separation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to make this better, but I don't have to worry about the readability of my code very often so I have no clue how well I did.
Fix regular expression period escapes
Is there any desire for the contents of this PR if the code (and possibly commit history) were cleaned up further? |
@kyledrake It appears as if @jimmiehansson is no longer involved with this project with a few years since his last interaction. Would you have any oppinion on merging these features later if I were to clean up the formatting and commit history of my contributions? Specific api calls added: |
There are many api features that are supported by the REST API, but not the NodeJS module. This pull request adds those features, along with a few extras. (Warning, download and pull have a high likelihood of being buggy.)