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

make tests for custom token() usage #127

Closed
wants to merge 4 commits into from
Closed

Conversation

bmeck
Copy link

@bmeck bmeck commented Oct 11, 2016

Superseding #124

  • fixes parsing of empty arguments via :x[]
  • makes token functions always be given the same length of arguments

Makes tests for custom usage of morgan.token from reading the docs.

@bmeck bmeck mentioned this pull request Oct 11, 2016
@bmeck bmeck force-pushed the token-tests branch 2 times, most recently from d41a5d5 to 6536b90 Compare October 11, 2016 13:46
@dougwilson
Copy link
Contributor

Awesome, will take a closer look soon :) A quick look shows that there are some comments from the previous PR that were missed on this one, so getting those addressed would help, otherwise I'll spend some time to write them back on this PR as well :) A few that jump out: #124 (comment) , #124 (comment) , #124 (comment) , #124 (comment) , #124 (comment) , #124 (comment) , #124 (comment) , #124 (comment)

@bmeck
Copy link
Author

bmeck commented Oct 12, 2016

Pushed changes, but opposed to DRY change.

On Tue, Oct 11, 2016 at 9:48 PM, Douglas Wilson notifications@github.com
wrote:

Awesome, will take a closer look soon :) A quick look shows that there are
some comments from the previous PR that were missed on this one, so getting
those addressed would help, otherwise I'll spend some time to write them
back on this PR as well :) A few that jump out: #124 (comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment) , #124
(comment)
#124 (comment)


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#127 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOUo3ZvCkKTJu19X7a5d9cda9eoxAXSks5qzEplgaJpZM4KTpPP
.

@dougwilson
Copy link
Contributor

but opposed to DRY change.

Please make the change; in the end, I have yo maintain the tests and the changes should be consistent with all the other code I am maintaining.

@bmeck
Copy link
Author

bmeck commented Oct 12, 2016

@dougwilson going to simply drop this. done.

@bmeck bmeck closed this Oct 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants