-
Notifications
You must be signed in to change notification settings - Fork 214
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
test: lint Ava tests #4981
test: lint Ava tests #4981
Conversation
6d92777
to
a3835c2
Compare
Wrinkle with If we're going to have an eslint config that gets imported by packages outside this repo then I suppose it should be minimal and very general. Then more specific ones that can be opted into. For example, sdk and dapp. The ava linting would only be on the 'sdk' one. So I'll change this PR to
|
a3835c2
to
6c2e3a4
Compare
6c2e3a4
to
2e359df
Compare
07ab914
to
3914c02
Compare
3914c02
to
950a973
Compare
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.
with caveats, as discussed.
// error: code = NotFound desc = account agoric1mhu... not found | ||
// Sometimes I can get this test to work alone, but not | ||
// if run with the test above. | ||
// TODO: https://github.com/Agoric/agoric-sdk/issues/6766 |
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.
Should this PR
closes: #6766
?
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.
sadly, no. It's still failing https://github.com/Agoric/agoric-sdk/actions/runs/5867547993/job/15908602617?pr=4981
I'll revert this change
packages/solo/src/start.js
Outdated
@@ -1,4 +1,5 @@ | |||
// @ts-check | |||
/* eslint-disable no-lone-blocks */ |
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.
Is this required because of the switch/case? If so, I think the rule is being too aggressive. I wouldn't want to have to add this caveat every time I use swtch/case. Doesn't our style recommend using braces with switch/case? (It should, IMHO.)
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.
Remnant of an earlier revision. I'll remove
@@ -0,0 +1,99 @@ | |||
import { Buffer } from 'buffer'; | |||
import { makeB0ID } from './util.js'; |
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.
remark: Good extraction!
950a973
to
7347734
Compare
I think |
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.
This all looks quite reasonable to me.
I don't know why it's a good rule to enforce, but I've found that my tastes don't seem to prevail unless I can show why a proposed rule would get in the way. I asked enough questions to convince myself that it doesn't prevent anything that I'm aware of doing. @FUDCo is there something that you occasionally do that this would grouse about? |
I think the example you already cited of switch cases is compelling by itself, but also: sometimes lone blocks are useful for expository purposes to highlight functionally distinct parts of a function, much as blank lines are used. Also, I've occasionally found them useful to control what gets captured by a closure -- sometimes optimizations inside the JS engine can result in more stuff getting captured by a closure than you want or than is lexically apparent from reading the code; arguably this latter case is more about working around JS engine issues, but real nonetheless. |
closes: #4941
Description
Motivation in #4941.
use-test
due to Allow importing AVA asanyTest
in TypeScript files avajs/eslint-plugin-ava#294 (comment)no-lone-blocks
that started erroring, I suppose because a dep that was updated caught more cases. Alternately we could disable the rule.Security Considerations
--
Documentation Considerations
--
Testing Considerations
--