Skip to content

Conversation

@blackk-foxx
Copy link

Incorporates ideas from #217, #218 and #219.

Copy link
Author

Choose a reason for hiding this comment

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

Are hints needed for this one? I'm open to suggestions...

Comment on lines 3 to 4
-- TODO: Add the appropriate SELECT statement here
;
Copy link
Author

Choose a reason for hiding this comment

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

If the user runs the tests on this stub file, it will result in syntax errors. There would also be syntax errors if any of the CREATE TABLE stubs are left unmodified, which would make it unlikely for the user to solve each task individually, in TDD fashion. Maybe it would be better to include a SELECT stub in each, like

Suggested change
-- TODO: Add the appropriate SELECT statement here
;
-- TODO: Update the SELECT statement as needed here
SELECT "TODO"
;

Other thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Hard agree. The stub should be valid SQL. Maybe drop the TODO comment and just do something like SELECT "Replace this SELECT statement with the correct SELECT statement to solve this task"

Copy link
Author

Choose a reason for hiding this comment

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

Done.


## The basics

The anatomy of a basic `SELECT` statement is as follows:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The anatomy of a basic `SELECT` statement is as follows:
The anatomy of a basic `SELECT` statement is as follows.

Copy link
Author

Choose a reason for hiding this comment

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

Why not :?

Copy link
Member

Choose a reason for hiding this comment

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

This is a complete sentence and complete idea as is.

Copy link
Author

Choose a reason for hiding this comment

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

I agree it's a complete sentence, but I don't agree that it's a complete idea. I think the information following "as follows" is essential to understanding the sentence, hence the colon. Anyway, if there is an existing convention to use a period after "as follows," I will follow it. Otherwise, I would prefer to use a colon.

.mode markdown
.output user_output.md
.read ./intro-select.sql
.shell rm -f ./results.db
Copy link
Member

Choose a reason for hiding this comment

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

Is this line needed if the save overrides?

Copy link
Author

Choose a reason for hiding this comment

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

Not necessary, but I thought it would be nice to leave the workspace clean.

Copy link
Member

Choose a reason for hiding this comment

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

The immediate next step writes this file, though. How does this result in a cleaner workspace?

Copy link
Author

Choose a reason for hiding this comment

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

D'oh!

Copy link
Author

Choose a reason for hiding this comment

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

Moved the removal to the end of the file.

.output

-- Report results
.shell bash ./report-results.sh results.db
Copy link
Member

Choose a reason for hiding this comment

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

Thoughts on dropping the sh extension? This isn't a sh script.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Should it be .bash, or no extension?

Copy link
Member

Choose a reason for hiding this comment

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

Personally I'd go with no extension.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

else
(got | rows | tostring) as $got_rows
| (expected | rows | tostring) as $expected_rows
| "With columns \($got_columns)\nexpected \($expected_rows)\nbut got \($got_rows)"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| "With columns \($got_columns)\nexpected \($expected_rows)\nbut got \($got_rows)"
| "With columns \($got_columns)\nexpected row \($expected_rows)\nbut got \($got_rows)"

To match the column error

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@blackk-foxx blackk-foxx requested a review from IsaacG December 17, 2025 20:05
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