-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[components docs] Flesh out existing code location docs, test #27604
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
Conversation
try: | ||
actual_output = ( | ||
subprocess.check_output( | ||
f'{cmd} && echo "PWD=$(pwd);"', shell=True, stderr=subprocess.STDOUT |
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.
Using &&
will only echo the PWD if the command succeeds. To ensure the PWD is always captured for proper directory tracking, this should use ;
instead: f'{cmd}; echo "PWD=$(pwd);"'
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
644be30
to
43e4f0f
Compare
except subprocess.CalledProcessError as e: | ||
print(f"Ran command {cmd}") # noqa: T201 | ||
print("Got output:") # noqa: T201 | ||
print(e.output.decode("utf-8").strip()) # noqa: T201 | ||
raise e | ||
|
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.
The actual_output
variable needs to be assigned before the error handling block exits. When an exception occurs, the code will raise a NameError
when trying to access actual_output
in the subsequent lines. Consider assigning e.output.decode('utf-8').strip()
to actual_output
before printing and raising the exception.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
...pets/docs_beta_snippets_tests/snippet_checks/guides/components/my-existing-project/README.md
Outdated
Show resolved
Hide resolved
...ppets/docs_beta_snippets_tests/snippet_checks/guides/components/my-existing-project/setup.py
Outdated
Show resolved
Hide 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.
one comment but otherwise lgtm
@@ -6,6 +6,9 @@ extend = "../pyproject.toml" | |||
# Shorter line length for docs snippets for better browser formatting. | |||
line-length = 88 | |||
|
|||
# Ignore a specific file | |||
extend-exclude = ["docs_beta_snippets/guides/components/existing-project/6-initial-definitions.py"] |
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.
Much prefer to ignore a file by putting:
# noqa
At the top then having this setting
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.
a211d2b
to
95cc4bb
Compare
…r-io#27604) ## Summary Adds a section to the 'existing project' docs about adding to `pyproject.toml` (necessary for `dg` cli to work) alongside optionally configuring a `uv` environment. This was after playing around trying to port a DOP project, I found some of these steps not immediately obvious & had a hard time setting up a new venv. Places the existing + new contents under integration test.
## Summary Adds a section to the 'existing project' docs about adding to `pyproject.toml` (necessary for `dg` cli to work) alongside optionally configuring a `uv` environment. This was after playing around trying to port a DOP project, I found some of these steps not immediately obvious & had a hard time setting up a new venv. Places the existing + new contents under integration test.
…r-io#27604) ## Summary Adds a section to the 'existing project' docs about adding to `pyproject.toml` (necessary for `dg` cli to work) alongside optionally configuring a `uv` environment. This was after playing around trying to port a DOP project, I found some of these steps not immediately obvious & had a hard time setting up a new venv. Places the existing + new contents under integration test.
Summary
Adds a section to the 'existing project' docs about adding to
pyproject.toml
(necessary fordg
cli to work) alongside optionally configuring auv
environment.This was after playing around trying to port a DOP project, I found some of these steps not immediately obvious & had a hard time setting up a new venv.
Places the existing + new contents under integration test.