search

alexa-js / alexa-app-server

An Alexa app server for alexa-app.
MIT License
403 stars 116 forks source link

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

Closed tejashah88 closed 7 years ago

dblock commented 7 years ago

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?

mreinstein commented 7 years ago

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

dblock commented 7 years ago

This can go in the next release, no rush.

dblock commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

If you don't mind looking/merging https://github.com/alexa-js/alexa-app-server/pull/72 first before it becomes a big conflict ...

dblock commented 7 years ago

Merged via https://github.com/alexa-js/alexa-app-server/commit/2be0f024cacb4318a96bc2ffa93faf24d49831aa.