-
-
Notifications
You must be signed in to change notification settings - Fork 526
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
CLI command for reflog #7032
CLI command for reflog #7032
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.
Looking good so far! Mostly some minor comments and some clarification on how Dolt's reflog is a little different than Git's and has some different requirements.
There are also some comments about how the analyzer processes queries that you probably haven't seen before. I'm happy to explain more about how that works if you wanna know more details there or if any of those comments just don't make sense.
ae2a9b8
to
5d24edd
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.
Looking very, very close now! Just a couple of things to follow up on. Hit me up if the analyzer-related comments don't make sense and I'm more than happy to explain in more detail.
@@ -135,6 +135,7 @@ SKIP_SERVER_TESTS=$(cat <<-EOM | |||
~cli-hosted.bats~ | |||
~profile.bats~ | |||
~ls.bats~ | |||
~reflog.bats~ |
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.
(minor) It would be nice if we could somehow document why we're skipping some of these. It'll be hard to remember in the future. I know you mentioned we couldn't run reflog.bats
through the local-remote test because it depends on dolt gc
which won't there yet. That would be a nice note to include, but I'm not sure this format would allow 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.
Nice work! 🙌🏻 Just two really small things I noticed that should be super quick to fix before you merge this one into main.
tabId sql.TableId | ||
colset sql.ColSet |
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.
These two members don't seem to be used anywhere, so should be removed.
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.
These were added in a separate PR, looks like they're being used a couple lines down.
if rltf.refExpr != nil { | ||
return rltf.refExpr.Resolved() | ||
for _, expr := range rltf.refAndArgExprs { | ||
return expr.Resolved() |
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 don't want to always return on the first expression – you need to check that all the expressions are resolved.
You probably want something like if expr.Resolved() == false { return false }
instead.
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
Adds the CLI command
dolt reflog
which displays results form thedolt_reflog()
table function