Skip to content
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

Rewrite the dolt show implementation. #8239

Merged
merged 4 commits into from
Aug 12, 2024
Merged

Rewrite the dolt show implementation. #8239

merged 4 commits into from
Aug 12, 2024

Conversation

nicktobey
Copy link
Contributor

dolt show had an issue where it would not correctly display the SerialMessage for commits if provided with a hash. This came about as part of a refactor to make dolt show not depend on the env.DoltEnv object, when only exists on locally running servers, and not when connected to a remote server. Unfortunately, it looks like that refactor didn't actually remove the dependency either, as DoltEnv was still used in every possible invocation of dolt show

To get it working, I essentially rewrote the implementation of dolt show in such a way that it now actually only uses DoltEnv when it can't get the necessary information from a running server: Basically, if we need to display SerialMessages or resolve branch names, we still rely on a locally running server. This can likely be improved in the future. But calls like dolt show #hash should now work against remote servers.

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
af49473 ok 5937457
version total_tests
af49473 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
cd31973 ok 5937457
version total_tests
cd31973 5937457
correctness_percentage
100.0

@nicktobey nicktobey force-pushed the nicktobey/show-fix branch from cd31973 to 727049a Compare August 9, 2024 15:19
@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
727049a ok 5937457
version total_tests
727049a 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
93eeb97 ok 5937457
version total_tests
93eeb97 5937457
correctness_percentage
100.0

@nicktobey nicktobey force-pushed the nicktobey/show-fix branch from 93eeb97 to b36f3d4 Compare August 9, 2024 17:53
@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
b36f3d4 ok 5937457
version total_tests
b36f3d4 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
26043fd ok 5937457
version total_tests
26043fd 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
f132884 ok 5937457
version total_tests
f132884 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
e8b2fe7 ok 5937457
version total_tests
e8b2fe7 5937457
correctness_percentage
100.0

@nicktobey nicktobey requested a review from macneale4 August 10, 2024 17:38
Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely better than what we started with. I'm OK to ship this, but I do think we would be well served to seperate out the admin interactions from the common user requirements. Ultimately dolt show should be fully executable over SQL.

continue
} else {
// Hash is a commit
err = printCommit(queryist, sqlCtx, opts, commitInfo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this naming of the functions in this file could be updated to be more expressive of what they are doing. printCommit is actually performing some calls over the queryist, while printRawValue is only operating on local files. With some much packed into "print" functions, it makes it hard to read the code.

I think a larger change which would make sense would to separate out the local dEnv cases. Put those in another command. Leaves much less cognitive overhead about how this command actually operates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to "fetchAndPrintCommit"

@nicktobey
Copy link
Contributor Author

As discussed offline, I totally agree about separating out the low-level admin and high-level user functionality of this command. But this is better than before and fixes multiple issues. It's a step in the right direction.

@coffeegoddd
Copy link
Contributor

@nicktobey DOLT

comparing_percentages
100.000000 to 100.000000
version result total
722c0c0 ok 5937457
version total_tests
722c0c0 5937457
correctness_percentage
100.0

@nicktobey nicktobey merged commit 2d857a3 into main Aug 12, 2024
21 checks passed
@nicktobey nicktobey deleted the nicktobey/show-fix branch August 12, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants