Skip to content
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

Share VS Code settings/extensions nicely #57671

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@samestep
Copy link
Contributor

@samestep samestep commented May 5, 2021

This is a second attempt at #51214. It should achieve the same goals with (as far as I can tell) no disadvantages, but the advantages are a bit less pronounced than in the more dictatorial approach that #51214 took:

  • Unfortunately, I was unable to figure out how to include the mypy configuration given in the docstring of tools.mypy_wrapper.main, because as @walterddr pointed out, "${env:HOME}/miniconda3/envs/pytorch/bin/python" is not guaranteed to be correct on everyone's machine:

    {
      "python.linting.enabled": true,
      "python.linting.mypyEnabled": true,
      "python.linting.mypyPath": "${env:HOME}/miniconda3/envs/pytorch/bin/python",
      "python.linting.mypyArgs": [
        "${workspaceFolder}/tools/mypy_wrapper.py"
      ]
    }

    Importantly, this does not work:

    "python.linting.mypyPath": "${workspaceFolder}/tools/mypy_wrapper.py"

    This is because VS Code does not run the given mypy command inside of the user's specified virtual environment, so for instance, on my system, setting the mypy command to directly call tools/mypy_wrapper.py results in using mypy 0.782 instead of the correct mypy 0.812.

    Sadly, this does not work either, although I'm not sure why:

    {
      "python.linting.mypyPath": "${config:python.pythonPath}",
      "python.linting.mypyArgs": [
        "${workspaceFolder}/tools/mypy_wrapper.py"
      ]
    }
  • As a result, git clean -fdx; tools/vscode_settings.py still results in some loss of useful configuration.

One other thing to note: as .vscode/settings_recommended.json shows, there are some configuration sections that only take effect within the context of a "[language]", so currently, if a dev already has one of those settings, it would be entirely overwritten by tools/vscode_settings.py rather than a graceful merge. This could probably be fixed by using a deep merge instead of the current shallow merge strategy.

Test plan:

If you want, you can typecheck the small script added by this PR (no output is expected):

tools/mypy_wrapper.py $PWD/tools/vscode_settings.py

You can also try running it to update your own VS Code workspace settings:

tools/vscode_settings.py

This should have minimal impact on your existing tools/settings.json file other than enabling the few explicitly recommended settings (e.g. it should not reorder or remove any of your existing settings).

@samestep samestep requested a review from May 5, 2021
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 5, 2021

💊 CI failures summary and remediations

As of commit 4971e68 (more details on the Dr. CI page):


💚 💚 Looks good so far! There are no failures yet. 💚 💚


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 5, 2021

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

1 similar comment
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 5, 2021

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

@samestep
Copy link
Contributor Author

@samestep samestep commented May 5, 2021

ah wait, I have an idea for how to solve the mypy thing

Loading

@samestep
Copy link
Contributor Author

@samestep samestep commented May 5, 2021

ah wait, I have an idea for how to solve the mypy thing

it only builds on this and might have some drawbacks, though, so I'll do that in a followup PR if this one is merged

Loading

.gitignore Outdated Show resolved Hide resolved
Loading
@samestep samestep requested a review from May 5, 2021
@samestep samestep requested review from malfet and May 5, 2021
.vs
/.vscode/*
!/.vscode/extensions.json
Copy link
Contributor

@driazati driazati May 5, 2021

git clean -xfd would still blow away vscode settings right? Maybe we should encourage people to use python setup.py clean instead

Loading

Copy link
Contributor Author

@samestep samestep May 5, 2021

it already does that even before this PR but yeah, that sounds like a good idea to me; in that case could you open a PR to replace git clean -xfd with python setup.py clean in CONTRIBUTING.md?

Loading

malfet
malfet approved these changes May 5, 2021
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 5, 2021

@samestep has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented May 5, 2021

@samestep merged this pull request in e5179e9.

Loading

@samestep samestep deleted the vscode-share-nicely branch May 5, 2021
krshrimali pushed a commit to krshrimali/pytorch that referenced this issue May 19, 2021
Summary:
This is a second attempt at pytorch#51214. It should achieve the same goals with (as far as I can tell) no disadvantages, but the advantages are a bit less pronounced than in the more dictatorial approach that pytorch#51214 took:

- Unfortunately, I was unable to figure out how to include [the `mypy` configuration given in the docstring of `tools.mypy_wrapper.main`](https://github.com/pytorch/pytorch/blob/7115a4b8707115b81417481fb6240617ae934806/tools/mypy_wrapper.py#L81-L89), because as walterddr pointed out, `"${env:HOME}/miniconda3/envs/pytorch/bin/python"` is not guaranteed to be correct on everyone's machine:
  ```json
  {
    "python.linting.enabled": true,
    "python.linting.mypyEnabled": true,
    "python.linting.mypyPath": "${env:HOME}/miniconda3/envs/pytorch/bin/python",
    "python.linting.mypyArgs": [
      "${workspaceFolder}/tools/mypy_wrapper.py"
    ]
  }
  ```

  Importantly, this does not work:
  ```json
  "python.linting.mypyPath": "${workspaceFolder}/tools/mypy_wrapper.py"
  ```
  This is because VS Code does not run the given `mypy` command inside of the user's specified virtual environment, so for instance, on my system, setting the `mypy` command to directly call `tools/mypy_wrapper.py` results in using `mypy 0.782` instead of the correct `mypy 0.812`.

  Sadly, [this](https://code.visualstudio.com/docs/editor/variables-reference#_configuration-variables) does not work either, although I'm not sure why:
  ```json
  {
    "python.linting.mypyPath": "${config:python.pythonPath}",
    "python.linting.mypyArgs": [
      "${workspaceFolder}/tools/mypy_wrapper.py"
    ]
  }
  ```

- As a result, `git clean -fdx; tools/vscode_settings.py` still results in some loss of useful configuration.

One other thing to note: as `.vscode/settings_recommended.json` shows, there are some configuration sections that only take effect within the context of a `"[language]"`, so currently, if a dev already has one of those settings, it would be entirely overwritten by `tools/vscode_settings.py` rather than a graceful merge. This could probably be fixed by using a deep merge instead of the current shallow merge strategy.

Pull Request resolved: pytorch#57671

Test Plan:
If you want, you can typecheck the small script added by this PR (no output is expected):
```sh
tools/mypy_wrapper.py $PWD/tools/vscode_settings.py
```
You can also try running it to update your own VS Code workspace settings:
```sh
tools/vscode_settings.py
```
This should have minimal impact on your existing `tools/settings.json` file other than enabling the few explicitly recommended settings (e.g. it should not reorder or remove any of your existing settings).

Reviewed By: malfet

Differential Revision: D28230390

Pulled By: samestep

fbshipit-source-id: 53a7907229e5807c77531cae4f9ab9d469fd7684
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment