Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

files add, cat, get core + cli offline #193

Closed
wants to merge 3 commits into from
Closed

Conversation

nginnever
Copy link
Member

No description provided.

@jbenet jbenet added the status/in-progress In progress label May 5, 2016
throw new Error('Error: Argument \'path\' is required')
}

s = fs.statSync(path)
Copy link
Member

Choose a reason for hiding this comment

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

You could declare s here where it's needed instead of at the top

if (s.isDirectory()) {
throw new Error('Error: ' + process.cwd() + ' is a directory, use the \'-r\' flag to specify directories')
}
}
Copy link
Member

Choose a reason for hiding this comment

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

What is the goal of this chunk of code? Can it be extracted to a function with a good name? At first glance seems madness to me :D

Copy link
Member Author

@nginnever nginnever May 5, 2016

Choose a reason for hiding this comment

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

It's to check for all of the inputs you could give to add command. variations i could think of are...

  1. "." add the cwd but throw error for no recursion flag
  2. "." -r add the cwd
  3. "/some/path" but throw error for no recursion
  4. "/some/path" -r
  5. No path, throw err
  6. filename.type

I could make a checkpath() function but i'm not sure how that would simplify any of the logic

Copy link
Member

Choose a reason for hiding this comment

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

Creating a function and adding that as a comment describing what it is doing would be considerably much better.

@daviddias
Copy link
Member

CR'ed 👍 left some comments that can be applied through the code. Cool that things are going :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants