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

Fixed the endpoint message to use app.name to match the endpoint route #76

Merged
merged 6 commits into from Feb 20, 2017

Conversation

Projects
None yet
3 participants
@zqylur
Copy link
Contributor

commented Feb 17, 2017

A minor thing, but something that stymied me for a while when I started using the package. The "loaded app at endpoiont" message is inaccurate if the package.json name and the name used in the Alexa app constructor are different. Changed the message to use the app name, which is where the endpoint actually is.

Happy to flip this as well, and change the router to use the package.json name.

@@ -5,6 +5,8 @@
* [#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).
* [#72](https://github.com/alexa-js/alexa-app-server/pull/72): Use `path.join` for constructing relative paths - [@dblock](https://github.com/dblock).
* [#74](https://github.com/alexa-js/alexa-app-server/pull/74): Added locale selector to test page - [@siedi](https://github.com/siedi).
* Changed endpoint message to use app name to match route

This comment has been minimized.

Copy link
@dblock

dblock Feb 17, 2017

Collaborator

Would you be so kind to make this look similar to other lines? Everything is on the same line, there's a -, a period at the end, etc.

This comment has been minimized.

Copy link
@zqylur

zqylur Feb 18, 2017

Author Contributor

Fixed the formatting and added the PR link.

@@ -128,7 +128,7 @@ var appServer = function(config) {
postRequest: self.config.postRequest
});

var endpoint = path.posix.join(normalizedRoot, pkg.name);
var endpoint = path.posix.join(normalizedRoot, app.name);
self.log(" loaded app [" + pkg.name + "] at endpoint: " + endpoint);

This comment has been minimized.

Copy link
@dblock

This comment has been minimized.

Copy link
@zqylur

zqylur Feb 18, 2017

Author Contributor

So I added the test, with a new entry in the "invalid_examples" folder, and added the assertion in the existing invalid examples test. I wasn't sure where it fit the best, so I'm happy to restructure if you think there's a better spot for it.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2017

The test feels a bit contrived or at least misplaced. First, it lives inside a test suite that is ensuring that invalid apps are loaded, it should be a standalone check all by itself. Second, this is a perfectly valid app, no? Maybe it doesn't belong in invalid_examples?

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 18, 2017

Let me take the valid/invalid app back - I see how you wouldn't want the names to be the way they are setup, so I am cool with the app being where it is. Just split the test about the logger warn message from the tests that ensure invalid apps are loaded at all.

@zqylur

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

Do you prefer the test to have its own file? Or just a new test in the "test-examples-server-app-loading-fail-checks" file?

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2017

I don't have a strong preference.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Feb 20, 2017

Looks good, merging.

@dblock dblock merged commit 94dd0a7 into alexa-js:master Feb 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 95.57%
Details
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.