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

enabled strictHeaderCheck #51

Merged
merged 4 commits into from Jan 31, 2017

Conversation

Projects
None yet
4 participants
@mreinstein
Copy link
Contributor

commented Jan 31, 2017

#50

@mreinstein mreinstein changed the title enabled strictHeaderCheck #50 enabled strictHeaderCheck Jan 31, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2017

Coverage Status

Coverage remained the same at 96.023% when pulling b28d467 on mreinstein:master into 3260bef on alexa-js:master.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

Needs changelog and a test please?

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2017

Coverage Status

Coverage remained the same at 96.023% when pulling b84feda on mreinstein:master into 3260bef on alexa-js:master.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

Much better. Still needs a CHANGELOG line.

I also winder whether we should add an UPGRADING document that describes breaking changes. Like this one. Feel free to obviously.

@mreinstein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2017

too much process.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

LOL @mreinstein just trying to have bewildered users wondering why suddenly something returns 401 instead of 200 when they upgrade!

@mreinstein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2017

just trying to have bewildered users wondering why suddenly something returns 401 instead of 200 when they upgrade!

Better instead to have a broken alexa-app-server package that isn't capable of passing alexa certification? ;) Because that's the current situation.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

Not trying to be difficult. Are you saying a changelog line is too much process? I can merge this as is, but I will be adding that CHANGELOG line next myself ;(

@mreinstein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2017

I know you're not trying to be difficult. And I'm not saying that a changelog in itself is a bad idea. It just feel like a lot more process is in place now.

@mreinstein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2017

2 issues I have with the current changelog:

a ) creating the changelog first, and then doing the release seems backwards to me. for one thing it's impossible to know what the version number will be, when the changes aren't done piling in. It's also confusing at first glance because when I look at a changelog I expect the top item to be the current release, not the upcoming release.

b) this isn't so much with the changelog itself but more with shipping modules. It's better to ship small changes frequently rather than piling up tons of stuff in any given release. There is virtually no costs to having additional module versions being published.

these are both hovering in the realm of opinion but I'd rather see quick agile releases that don't try to package too much at once.

@tejashah88

This comment has been minimized.

Copy link
Member

commented Jan 31, 2017

I guess it's bound to happen at some point. As all these libraries grow and mature, we do eventually need more process in order to avoid development of such libraries to be a nightmare. IMO, I don't like more process but to an extent, it's necessary for the future of this library, and maybe others.

@mreinstein

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2017

in order to avoid development of such libraries to be a nightmare

I guess that's where we differ in opinion. I believe these alexa modules are already quite mature and stable. They are in maintenance mode. They don't have extensive features that need to be added, and in fact I believe we should be resisting the urge to massively overhaul or bloat these with new capabilities.

Not having extensive process did not prevent these modules from being successful or stable, which is why most people are using them in their projects.

@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

I agree with most things that you say @mreinstein. And we should discuss process in a separate issue, feel free to open one and propose something better.

mreinstein added some commits Jan 31, 2017

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2017

Coverage Status

Coverage remained the same at 96.023% when pulling e8680a2 on mreinstein:master into 3260bef on alexa-js:master.

@coveralls

This comment has been minimized.

Copy link

commented Jan 31, 2017

Coverage Status

Coverage remained the same at 96.023% when pulling e8680a2 on mreinstein:master into 3260bef on alexa-js:master.

@dblock dblock merged commit 9e1b2a9 into alexa-js:master Jan 31, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 96.023%
Details
@dblock

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2017

Merged, thanks!

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.