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

Initial commit. #1

Merged
merged 5 commits into from Oct 8, 2017
Merged

Initial commit. #1

merged 5 commits into from Oct 8, 2017

Conversation

dkorolev
Copy link
Owner

@dkorolev dkorolev commented Oct 8, 2017

Hi @sompylasar, could you take a look? :-) Thanks!

Copy link
Collaborator

@sompylasar sompylasar left a comment

Choose a reason for hiding this comment

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

It should work like this, but...

@@ -1,2 +1,46 @@
# curlable

A simple wrapper to turn [stateless] command-line utilities into HTTP endpoints.
Copy link
Collaborator

Choose a reason for hiding this comment

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

"stateless" looks like a malformed Markdown link.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Except it's not.

README.md Outdated
You should see:

```
dima ~/github/dkorolev/curlable (dev) $ node curlable.js
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove "dima ~/github/dkorolev/curlable (dev) "

Copy link
Owner Author

Choose a reason for hiding this comment

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

You are just nitpicking. Let me do it in another commit, already work in progress.

README.md Outdated
```

```
dima ~ $ curl -d '2 ^ 10' localhost:8000 && curl -X DELETE localhost:8000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove "dima ~ "

Copy link
Owner Author

Choose a reason for hiding this comment

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

You are just nitpicking. Let me do it in another commit, already work in progress.


The supported command line parameters are:

1. `-c`: The external command to `curl`-ify, defaults to `bc -l`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default command makes no sense, better remove it (add explicit to the example).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Why does it make no sense? It's just to test 2 + 2 returns 4.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes no sense from the perspective of the user of this wannabe-useful npm package / cli tool.

Copy link
Owner Author

Choose a reason for hiding this comment

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

It makes it super-simple to test in IRL, w/ zero configuration. Why not keep it?

## Running the Tests

```
$ npm i
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better keep the long-hands in the documentation (npm install).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Why?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because that's what is listed in the npm documentation.

{
"name": "curlable",
"version": "0.0.1",
"description": "A simple wrapper to turn [stateless] command-line utilities into HTTP endpoints.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Square brackets look awkward here.

package.json Outdated
"type": "MIT"
}
],
"license": "MIT",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "license" is enough.

package.json Outdated
"bash",
"shell"
],
"main": "curlable.js",
Copy link
Collaborator

Choose a reason for hiding this comment

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

"main" should point to programmatically usable entry point (i.e. to lib/curlable.js); use "bin" to point to the command-line entry point.

@@ -0,0 +1,42 @@
'use strict';
Copy link
Collaborator

Choose a reason for hiding this comment

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

#!/usr/bin/env node and chmod +x

var app = express();
app.use(bodyParser.text({
type: '*/*'
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem to belong in here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Where should it belong? I can't parse the body otherwise.

@dkorolev
Copy link
Owner Author

dkorolev commented Oct 8, 2017

@sompylasar, thanks! PTAL @ 26a13d1.

@dkorolev
Copy link
Owner Author

dkorolev commented Oct 8, 2017

bin is an object

@sompylasar, why the fuck did npm i work then?! Ref. 580e73f

@dkorolev
Copy link
Owner Author

dkorolev commented Oct 8, 2017

@sompylasar, shall we merge this one and add one more feature today, in the next 15 minutes? :)

@dkorolev
Copy link
Owner Author

dkorolev commented Oct 8, 2017

Merging this one and creating one more, perhaps the last, PR.

@dkorolev dkorolev merged commit 863e281 into master Oct 8, 2017
@sompylasar
Copy link
Collaborator

@dkorolev Sure, you can merge.

I'd prefer to either leave it like this, or receive more precise instructions re. what's The Node Way.

I don't think it's "the node way", it's basic OOP plus limiting the number of possible states and transitions. RAII is C++ way, btw.

From the functionality standpoint, this may have been shaped as an express middleware (createMiddleware function which returns a (req,res,next) function).

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.

None yet

2 participants