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

Add test for optional route parameter #43

Merged
merged 4 commits into from
Jun 1, 2020

Conversation

smarts
Copy link
Contributor

@smarts smarts commented Dec 27, 2019

Test for #40

Copy link
Member

@3imed-jaberi 3imed-jaberi left a comment

Choose a reason for hiding this comment

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

Can you use const/let over var !!

It does not add anything to the set of tests but it can add to attracts attention to optional routes.

@smarts
Copy link
Contributor Author

smarts commented Jun 1, 2020

Sure. Both styles were in the code already, so @3imed-jaberi how would you feel about me doing a sweep and fixing it everywhere in this file?
On an unrelated note as a bit of constructive criticism: in the future, a simple . or ? is probably better. Using !! comes across a bit aggressive and could alienate contributors.

@niftylettuce
Copy link
Contributor

@smarts no worries - they didn't mean to alienate in any way, @3imed-jaberi is excited and happy to see your contributions! 🎉 🎉

@smarts
Copy link
Contributor Author

smarts commented Jun 1, 2020

Interesting, this test passes now. Looks like the bug has been fixed.

@niftylettuce niftylettuce merged commit bb74b12 into koajs:master Jun 1, 2020
@niftylettuce
Copy link
Contributor

thanks @smarts

@smarts
Copy link
Contributor Author

smarts commented Jun 1, 2020

Happy to help. Thanks to you @niftylettuce and @3imed-jaberi as well!

@3imed-jaberi
Copy link
Member

@smarts, sorry ... I didn't mean to insult or belittle or anything negative ... I just tried to provide a comment summarizing the situation in this PR and I wanted to say thank you for your contribution and that your work provides a good example ..

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.

3 participants