-
Notifications
You must be signed in to change notification settings - Fork 16
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
Added tree helpers #12
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.
Sorry for the late reply. I think these are okay overall, and thanks for adding tests, too!
tree_utils.go
Outdated
|
||
import "strings" | ||
|
||
func (n *Node) FindFocusedLeaf() *Node { |
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 it would be easier to understand if we introduced only the func(n *Node) bool { return n.Focused }
predicate. Can you do that and remove the FindFocusedLeaf
function?
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 mean something like this?:
// implementation
package i3
func IsFocused(n *Node) bool {
return n.Focused
}
....
// usage
package main
var n Node
...
n = GetBlahBlahNode()
n.FindFocused(i3.IsFocused())
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.
Yes, though now that I see it written out, it would merely be an accessor for the Focused field.
Maybe it would be better to not introduce this one after all. It’s just one line to write out, and fairly straight-forward.
Can you remove please?
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.
Done
tree_utils.go
Outdated
} | ||
|
||
func (n *Node) IsFloating() bool { | ||
if len(n.Floating) < 4 { |
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 conditional necessary? strings.HasSuffix
should be safe to call regardless the length of the string.
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.
Fixed
00f9ac2
to
968f135
Compare
968f135
to
b41c25a
Compare
Hi @stapelberg, thanks for approving :) |
It should: https://github.com/golang/go/wiki/Modules#how-to-upgrade-and-downgrade-dependencies You can try explicitly specifying the Or you can try |
import "i3" |
just to correct myself: go get go.i3wm.org/i3/v4@master import "go.i3wm.org/i3/v4" |
As we discussed in #11
there are a few helpers to extend the tree API.
Please take a look, maybe it is OK with the current project philosophy.