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

Fix for #68, added utility functions, more descriptive error messages #71

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
3 participants
@tejashah88
Copy link
Member

commented Feb 5, 2017

No description provided.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 5, 2017

First, we should make a hard rule not to take this kind of stuff without tests. If it helps, you can add a root property and test the output.

Secondly, manipulating paths this way is potentially error prone - path.normalize seems to be designed exactly for this purpose, so why not use it?

"mocha": "^2.3.4",
"chai": "^3.4.1",
"supertest": "^2.0.1",
"mocha": "^3.2.0",

This comment has been minimized.

Copy link
@dblock

dblock Feb 5, 2017

Collaborator

I previously downgraded some dependencies here to make things work with older versions of node. Surpried this works.

@mreinstein

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2017

use a module for this. Do not write custom code to deal with pathing, separators, etc.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2017

This can go in the next release, no rush.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2017

I realize this is used for cosmetic display purposes only, so I think we can live without tests for it. I still believe these urls should be combined with something like https://www.npmjs.com/package/url-join.

I fixed the places where we combine paths with + in #72.

@tejashah88

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

I was actually going to change the intent of the PR. Right now, we have a bunch of repetitive code, specifically with loading the modules and other files, and I was thinking that it'd make more sense to make a small utils file that contains any/all helper functions. I'll update the PR in a couple of hours.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2017

If you don't mind looking/merging #72 first before it becomes a big conflict ...

@tejashah88 tejashah88 changed the title Fix #68: Deal with multiple slashes in normalizedRoot Fix #68: Deal with multiple slashes in normalizedRoot, added utility functions Feb 7, 2017

@tejashah88 tejashah88 changed the title Fix #68: Deal with multiple slashes in normalizedRoot, added utility functions Fix for #68, added utility functions, more descriptive error messages Feb 7, 2017

index.js Outdated
@@ -26,13 +26,13 @@ var appServer = function(config) {
app_dir: 'apps'
};

self.config = defaults(config, defaultOptions);
self.config = utils.defaults(config, defaultOptions);

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

This is a wrapper for a require, seems unnecessary as it provides no additional value.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 7, 2017

Author Member

I just moved defaults in utils.js since it is technically a 'utility' function. I can move it back though.

index.js Outdated
if (!pkg || !pkg.main || !pkg.name) {
self.error(" failed to load " + package_json);
return;
}

var main = fs.realpathSync(path.join(app_dir, dir, pkg.main));
if (!fs.existsSync(main) || !fs.statSync(main).isFile()) {
var main = path.join(__dirname, app_dir, dir, pkg.main);

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

This inserts __dirname, which is a change in behavior from a previous version. At the very least it needs to be documented in both README and UPGRADING.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 8, 2017

Author Member

Yea, I did do some testing and it is kind of a breaking change. I'll open a new issue in the meantime.

index.js Outdated
});
};
server_files(server_dir).forEach(function(file) {
file = fs.realpathSync(path.join(server_dir, file));
file = path.join(__dirname, server_dir, file);

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

Same as above with __dirname.

index.js Outdated
var privateKey = fs.readFileSync(privateKeyFile, 'utf8');
var certificate = fs.readFileSync(certificateFile, 'utf8');
if (utils.isValidFile(privateKeyFile) && utils.isValidFile(certificateFile)) {
var privateKey = utils.readFile(privateKeyFile, 'utf8');

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

readFile doesn't take two parameters, the utf8 is implied.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 7, 2017

Author Member

Good catch. Thanks!

@@ -20,21 +20,23 @@
"license": "MIT",
"dependencies": {
"alexa-app": "^3.0.0",
"alexa-verifier-middleware": "^0.1.8",
"alexa-verifier-middleware": "^0.1.9",

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

I don't think this is a required change, the library works fine with 0.1.8, doesn't it?

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 8, 2017

Author Member

Actually, since alexa-app already uses alexa-verifier-middleware already, it will probably be removed.

"bluebird": "^3.4.7",
"body-parser": "~1.16.0",
"ejs": "~2.5.5",
"express": "~4.14.1",
"hotswap": "^1.1.0",
"lodash.defaults": "^4.2.0"
"lodash.defaults": "^4.2.0",
"lodash.endswith": "^4.2.1",

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

These aren't used, posix. is better.

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 7, 2017

Author Member

Good catch. Thanks!

var utils = require("../utils");

describe("Utilities testing", function() {
it("checks the integrity of the utilities", function() {

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

IMO this isn't a useful test. The actual calls matter, while the fact that these functions are declared seems superfluous.

chai.config.includeStack = true;
var utils = require("../utils");

describe("Utilities testing", function() {

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

In BDD you describe what "things are" and "how they behave", instead of what you're doing. So this should just be Utils.

]);
});

describe("File verification tests", function() {

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

isValidFile

This comment has been minimized.

Copy link
@tejashah88

tejashah88 Feb 8, 2017

Author Member

Do you mean that it should be describe("isValidFile", function() {?

This comment has been minimized.

Copy link
@dblock

dblock Feb 8, 2017

Collaborator

Yes, because it describes the functionality of that function. If you want to be fancy you can add a period in front of it :) Some people do #isValidFile or .isValidFile

utils.js Outdated

var defaults = require("lodash.defaults");

// Utility functions

This comment has been minimized.

Copy link
@dblock

dblock Feb 7, 2017

Collaborator

The comment here isn't helpful. It's pretty clear that utils declares utility functions. However you can document each individual function if you want.

@dblock

dblock approved these changes Feb 8, 2017

@@ -27,13 +27,13 @@ Modify the "Stable Release" section in [README.md](README.md). Change the text t
```
## Stable Release
You're reading the documentation for the stable release of alexa-app-server, 2.4.0.
You're reading the documentation for the stable release of alexa-app-server, 3.0.1.

This comment has been minimized.

Copy link
@dblock

dblock Feb 8, 2017

Collaborator

Since these are examples I never change them, but no big deal.

@@ -2,6 +2,7 @@

### 3.0.1 (Next)

* [#71](https://github.com/alexa-js/alexa-app-server/pull/71): Fixed [#68](https://github.com/alexa-js/alexa-app-server/issues/68), added utility functions, more descriptive error messages - [@tejashah88](https://github.com/tejashah88).

This comment has been minimized.

Copy link
@dblock

dblock Feb 8, 2017

Collaborator

What's important in changelog is to describe the actual fix to be helpful for the person reading it. So for example:

* [#71](https://github.com/alexa-js/alexa-app-server/pull/71), [#68](https://github.com/alexa-js/alexa-app-server/issues/68): Fixed log output containing multiple slashes - [@tejashah88](https://github.com/tejashah88).
@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 8, 2017

Merged via 2be0f02.

@dblock dblock closed this Feb 8, 2017

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