Add configurable port and browser behavior for autobuild#405
Add configurable port and browser behavior for autobuild#405conradolandia wants to merge 1 commit intospyder-ide:masterfrom
Conversation
✅ Deploy Preview for spyder-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
CAM-Gerlach
left a comment
CAM-Gerlach
left a comment
There was a problem hiding this comment.
While I certainly understand the motivation for making the port and open browser behavior configurable, and I'm not fundamentally opposed to using environment variables for doing that, every other configurable nox command we have (and we have many) does so via CLI options rather than env variables, so it seems inconsistent to treat nox -s autobuild as a special case and require long, verbose env vars to configure them. Furthermore, this is also inconsistent with our nox -s serve command, which exposes the exact same potentially configurable behavior (serving on a port and opening that in the browser) but lacks this novel configuration technique.
To be simpler and more consistent for users, at least for now instead of all this we should just read the config from CLI args (using the existing extract_option_values helper as necessary), and do so in nox -s serve as well, in addition to using a consistent default for both of them.
| SERVE_PORT = 5000 | ||
| AUTOBUILD_PORT = 8000 |
There was a problem hiding this comment.
Is there a legitimate reason why SERVE_PORT and AUTOBUILD_PORT need to be different here? Can they not just use a common SERVE_PORT, and the port for nox -s serve be configurable via the same mechanism as that for nox -s autobuild?
| return filenames | ||
|
|
||
|
|
||
| def env_var_to_bool(var_name, *, default): | ||
| """Convert an environment variable to bool with common false values.""" | ||
| value = os.environ.get(var_name) | ||
| if value is None: | ||
| return default | ||
| return value.strip().lower() not in {"0", "false", "no", "off"} | ||
|
|
||
|
|
||
| def extract_option_values(options, option_names, *, split_csv=False): |
There was a problem hiding this comment.
| return filenames | |
| def env_var_to_bool(var_name, *, default): | |
| """Convert an environment variable to bool with common false values.""" | |
| value = os.environ.get(var_name) | |
| if value is None: | |
| return default | |
| return value.strip().lower() not in {"0", "false", "no", "off"} | |
| def extract_option_values(options, option_names, *, split_csv=False): | |
| return filenames | |
| def extract_option_values(options, option_names, *, split_csv=False): |
Not needed anymore if we just use CLI args, like everything else in here does.
| autobuild_port = os.environ.get( | ||
| AUTOBUILD_PORT_ENV_VAR, str(AUTOBUILD_PORT) | ||
| ) | ||
| open_browser = env_var_to_bool( | ||
| AUTOBUILD_OPEN_BROWSER_ENV_VAR, default=True | ||
| ) |
There was a problem hiding this comment.
| autobuild_port = os.environ.get( | |
| AUTOBUILD_PORT_ENV_VAR, str(AUTOBUILD_PORT) | |
| ) | |
| open_browser = env_var_to_bool( | |
| AUTOBUILD_OPEN_BROWSER_ENV_VAR, default=True | |
| ) | |
| ports = extract_option_values(posargs, "--port")[0] | |
| autobuild_port = port[-1] if ports else SERVE_PORT | |
| open_browser = "--no-browser" not in posargs |
Just use CLI args for this instead
2 participants
Pull Request Checklist
Description of Changes
This PR adds
noxfileoptions so developers can choose the port and browser behavior forautobuildsessions.By default, the
autobuildsession now serves docs on port8000(instead of a random port) and opens the default browser. You can override this behavior with environment variables:This behavior is documented in
CONTRIBUTING.md.Motivation
This change makes
autobuildmore predictable while keeping it flexible. A fixed default port (8000, which is commonly used in local development setups) simplifies repeated local runs and integration with local tooling. Environment-variable overrides still allow custom setups, including alternate ports and workflows where opening a browser is not desired.