Re-organise sphinx configuration #1027
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Rather than do nothing, assume the user knows what they are doing and re-use the environment the language server itself is written in.
alcarney
force-pushed
the
reorg-sphinx-conf
branch
3 times, most recently
from
6fa098f to
0e4b4cf
Compare
This makes the server's initializationOptions the lowest priority setting, allowing for them to be used as fallback options specified by the client
alcarney
force-pushed
the
reorg-sphinx-conf
branch
from
0e4b4cf to
3cec2e8
Compare
This commit removes the following options
- ``esbonio.sphinx.envPassthrough``: For ages now, the language server
passes through all environment variables to the sphinx process so
this option has no use.
- ``esbonio.sphinx.pythonPath``: I think the original intent behind
this option was to allow you to override the version of the sphinx
agent.
However, the better way to do this is just use a different version
of ``esbonio``.
- ``esbonio.sphinx.cwd``: Use ``esbonio.sphinx.pythonCommand.cwd``.
- ``esbonio.sphinx.fallbackEnv``: This was a hack to allow the VSCode
extension to provide a default environment in the absence of any
user configuration.
From now on, clients should set ``esbonio.sphinx.pythonCommand`` in
the initializationOptions they send.
This commit also converts ``esbonio.sphinx.pythonCommand`` from a
simple list to an object in its own right.
- ``esbonio.sphinx.pythonCommand.command``: This list of strings
from the original ``esbonio.sphinx.pythonCommand.``
- ``esbonio.sphinx.pythonCommand.env``: A dictionary allowing you to
add/override environment variables passed to the sphinx process.
- ``esbonio.sphinx.pythonCommand.cwd``: A string allowing you to
change the directory in which the sphinx process is launched.
Now 95% of the time, the simple list of strings was sufficient so if
the server is given a list of strings for
``esbonio.sphinx.pythonCommand`` it will be automatically converted to
the object representation.
This batch of changes has unearthed a bunch of interesting timing issues during server start up. I was seeing an issue where the `esbonio.sphinx.pythonCommand` configuration was not being applied *only* in the case where I had one or more `*.rst` files open before restarting the server. I eventually realised that this was due to the `textDocument/didOpen` notification handlers winning the race with the `initialized` notification handler and creating a `SphinxConfig` configuration subscription **before** the additional structure hooks in the `SphinxManager` could be registered. This meant the promotion of `list[str]` values to `SubProcess` values was not happening for these early subscriptions. More broadly, this probably points to a need to reconsider the startup sequence at some point, but running this code during `initialize` (during which time other messages are forbidden) should be enough to paper over the issue for now.
Related to the previous commit, the server was declaring itself ready before all the `initialized` handlers of the features were processed. This commit therefore attempts to be more correct by delaying the resolution of the `ready` future until after the execution of these handlers. However, the previous commit also demonstrates how this already insufficient to ensure correct behaviour as other requests/notifications are free to be serviced concurrently with `initialized`. The "obvious" fix for this would be to move all setup code into the `initialize` handler, but there is at least one situation where this cannot be done and that is the configuration system. Initializing the configuration system involves making `workspace/configuration` requests to the client - which are forbidden until after the `initialize` result has been sent! As I write this, perhaps the other "obvious" solution is to have all of the message handlers in `esbonio/server/setup.py` consult the state of the `ready` future before proceeding... but for some reason (I can't quite articulate why) that doesn't feel like an ideal solution...
alcarney
force-pushed
the
reorg-sphinx-conf
branch
from
7d5fcf8 to
4293e66
Compare
alcarney
force-pushed
the
reorg-sphinx-conf
branch
12 times, most recently
from
29bf5f3 to
a353139
Compare
alcarney
force-pushed
the
reorg-sphinx-conf
branch
from
a353139 to
2dde21f
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Configuration changes
This PR removes the following options
esbonio.sphinx.envPassthrough: For ages now, the language server passes through all environment variables to the sphinx process so this option has no use.esbonio.sphinx.pythonPath: I think the original intent behind this option was to allow you to override the version of the sphinx agent. However, the better way to do this is just use a different version ofesbonio.esbonio.sphinx.cwd: Useesbonio.sphinx.pythonCommand.cwd.esbonio.sphinx.fallbackEnv: This was a hack to allow the VSCode extension to provide a default environment in the absence of any user configuration.From now on, clients should set
esbonio.sphinx.pythonCommandin the initializationOptions they send.This PR also converts
esbonio.sphinx.pythonCommandfrom a simple list to an object in its own right.esbonio.sphinx.pythonCommand.command: This list of strings from the originalesbonio.sphinx.pythonCommand.esbonio.sphinx.pythonCommand.env: A dictionary allowing you to add/override environment variables passed to the sphinx process. Closes Add ability to set envrionment variables for the sphinx process #469esbonio.sphinx.pythonCommand.cwd: A string allowing you to change the directory in which the sphinx process is launched.Now 95% of the time, the simple list of strings was sufficient so if the server is given a list of strings for
esbonio.sphinx.pythonCommandit will be automatically converted to the object representation.Behaviour changes
pythonCommandis given, instead of giving up the server will attempt to use the server's environment (see [Task] Improve Python interpreter selection step #736 (comment))initializationOptionshave gone from the highest priority to the lowest. This allows clients to provide default/fallback options, removing the need for explicit configuration options. Closes fallbackEnv outside of vscode #1024