-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
Init #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
|
||
``` | ||
warning.create(name, code, message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some time now I have been really enamored with named parameters. What do you think of this signature instead?:
warning.create(name, code, message) | |
warning.create({ name, code, message }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like to use options objects as parameters when some of those parameters are optional, given that here all of them are mandatory, I think we could keep them as is. Also, this "style" follows the same as the fastify-error
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they give clarity to the reader. For example, random bit of code:
const warning = fastifyWarning.create('foo', 'fst_foo', 'a warning')
vs
const warning = fastifyWarning.create({ name: 'foo', code: 'fst_foo', message: 'a warning' })
In my opinion, the second is easier to read by anyone, not just those who are already familiar with the API.
@@ -0,0 +1,143 @@ | |||
'use strict' | |||
|
|||
const test = require('ava') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not tap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from the requirement of using their CLI, I like the design of ava
, just wanted to try it a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That runner requirement is because it always transpiles the code being tested. It makes it unnecessarily difficult to debug things if needed.
Personally, I'd rather see consistency across all "official" modules. 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ava does not transpile the code anymore.
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
Co-authored-by: James Sumners <james@sumners.email>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Initial code.
Checklist
npm run test
andnpm run benchmark