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

Better type handling in harness, support COUNT(*) expressions and CAST #57

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented Nov 28, 2023

No description provided.

@zachmu zachmu requested a review from Hydrocharged November 28, 2023 02:16
Copy link
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

I know I say it in the comments but I'll mention it here again: I don't want to come across as being pedantic with the requests. I just want the codebase to remain consistent and as high-quality as possible for as long as we can, before we inevitably have to start working fast and hacking things to meet customer demand. The better the foundation, the easier we'll have it later on.

Preferably, all of this would be rules in some static analyzer, but I have no clue how to set that up, so it's PR comments for now. I probably should write some analyzer or linter or something, as this will break down once I'm no longer the only reviewer.

server/ast/expr.go Show resolved Hide resolved
server/ast/expr.go Outdated Show resolved Hide resolved
@zachmu zachmu requested a review from Hydrocharged November 28, 2023 22:55
@zachmu
Copy link
Member Author

zachmu commented Nov 28, 2023

I know I say it in the comments but I'll mention it here again: I don't want to come across as being pedantic with the requests. I just want the codebase to remain consistent and as high-quality as possible for as long as we can, before we inevitably have to start working fast and hacking things to meet customer demand. The better the foundation, the easier we'll have it later on.

Preferably, all of this would be rules in some static analyzer, but I have no clue how to set that up, so it's PR comments for now. I probably should write some analyzer or linter or something, as this will break down once I'm no longer the only reviewer.

I think all this is taken care of but github is still blocking merge, not sure why. Want to stamp?

Copy link
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

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

Much better!

@zachmu zachmu merged commit 5da75fd into main Nov 28, 2023
6 checks passed
@zachmu zachmu deleted the zachmu/exprs branch November 28, 2023 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants