-
Notifications
You must be signed in to change notification settings - Fork 1
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
Tutorial updates #117
Tutorial updates #117
Conversation
docs/source/tutorials/airbnb.rst
Outdated
|
||
$ sqlsynthgen make-stats --config-file config.yaml --force | ||
|
||
This will generate a file with the results of your sql queries from config.yaml and these results will be stored in the variable ``SRC_STATS``. |
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 would reword this. Something like: "With this command, the results of the query defined in config.yaml
will be stored in the src-stats.yaml
file. If you recreate the generators, stored in ssg.py
, you can verify that the query results are exposed via the SRC_STATS
variable.
docs/source/tutorials/airbnb.rst
Outdated
This will generate a file with the results of your sql queries from config.yaml and these results will be stored in the variable ``SRC_STATS``. | ||
From there they can be passed to your generators as arguments. | ||
|
||
In this example ``user_age_provider``, the ``query_results`` argument represents the results of the ``age_stats`` query and returns a random age value. |
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.
Just for clarity, I would reword it like: "Within the function user_age_provider
, defined in airbnb_generators.py
, the ..."
pyproject.toml
Outdated
@@ -25,6 +25,7 @@ sqlacodegen = "3.0.0rc1" | |||
asyncpg = "^0.27.0" | |||
greenlet = "^2.0.2" | |||
pymysql = "^1.1.0" | |||
pre-commit = {version = "^3.3.3", extras = ["dev"]} |
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.
Why was this dependency added?
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 asked Olajumoke to add it because we use pre-commit
checks and have a config file for it. This came up when I suggested that she run pre-commit install
, but she needed to install pre-commit first. It's unrelated to any of the other changes in this PR, just came up at the same time.
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.
Makes sense :)
@Olajumoke01 is looking good: I just made some minor suggestions. Also, if you still have them, it would be nice to add the complete |
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.
Some minor changes, that I flagged as comments.
Hey @cptanalatrist, Do you mean add them near to the end of the tutorial as an example or as separate files somewhere? |
Maybe you can add the files to the |
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.
Can you please rename the file to be all lowecase?
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.
Looks good to me! Only a minor renaming request.
only one thing, as discussed with Markus:
|
Updated version of the airbnb tutorial with missing code and commands. I think all the code in the tutorial should be working now. I added comments to the ssg manual edit file, to make it clearer which lines were added.