-
Notifications
You must be signed in to change notification settings - Fork 652
feat: move nil check to call sites #658
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
base: main
Are you sure you want to change the base?
Conversation
@rbuckton Can you take a look at this? |
@SoulPancake Sorry for jumping on here for a slightly tangential issue, but I noticed that nil checks are inconsistent throughout the codebase. For example, https://github.com/microsoft/typescript-go/blob/main/internal%2Fast%2Fast.go#L15-L31 type Visitor func(*Node) bool
func visit(v Visitor, node *Node) bool {
if node != nil {
return v(node)
}
return false
}
func visitNodes(v Visitor, nodes []*Node) bool {
for _, node := range nodes {
if v(node) {
return true
}
}
return false
}
|
@SaadiSave Agreed, I will add the nil check in the loop |
@SoulPancake is there a lint to enforce nil checks? |
Don't think so @SaadiSave |
This PR has gotten out of date as main changed. Could you update it, or close it if you don't plan on working on this anymore? |
I will update it asap @jakebailey |
699cd2d
to
fd4cf87
Compare
@jakebailey Done, Can you take a look at this? |
_submodules/TypeScript
Outdated
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.
You seem to have accidentally moved this during the merge/rebase. Do a git checkout main -- _submodules/TypeScript
to fix it.
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.
Hm, still not up to date, your main must not be up to date with upstream.
This addresses
// TODO(rbuckton): Move
node != niltest to call sites
@rbuckton Can you please review this
I can undo the stylistic whitespace changes in the internal/ast/utilities.gbut I think they're for the better, LMK what you think