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

[VSCODE] developer environment update on settings #9817

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented May 31, 2024

This PR is about update settings in VSCODE for improving our common dev environment accross OS

Challenges

The team develop on Windows and Unix environment, and there are some known difference like binary extension (deno.exe vs deno or quarto.cmd vs quarto

Also, we all have different workflow but we all use VSCODE (I believe). So it would be best to leverage VS CODE own settings feature to insure we use the same toolings (like extensions, which drives code practice like linting, formatting, tool configurations ...)

Context

Currently we have two settings file in repo

The project folder settings will alway be loaded and applied when opening the folder in VSCODE.

  • In VSCODE, it looks like this
    image
  • and in VSCODE settings UI, the project settings are identified as workspace
    image

The workspace file settings will be loaded only when workspace is opened (e.g. opening using code quarto-cli.code-workspace).

  • In VSCODE, it looks like this
    image
  • and in VSCODE settings UI, there is one more layer where workspace settings are now the code-workspace
    image

In the last example, this shows the order of settings

  • User settings are loaded
  • They are overriden by Workspace settings
  • Which are overriden by Project folder settings

Windows specific

Some settings we have can't work for windows

Main problem is that VSCODE does not support per-os settings. This is too bad, but we can't do what was done in launch.json to support multi settings

"windows": {
"runtimeExecutable": "${workspaceFolder}\\package\\dist\\bin\\tools\\x86_64\\deno.exe"
}

So what I do is that I don't use quarto-cli.code-workspace which contains the Unix specific settings, to have another workspace file that will set the settings for Windows.

The project folder settings .vscode/settings.json still applies, but quarto-cli.code-worspace do not, and I need to duplicate the settings with some modifications for windows.

More of a question: Conflicted settings ?

We have duplicate of deno.importMap settings

I don't know which one should be the right one for when we are in VSCODE developping, but it may be worth removing the one we don't one.

What this PR is about ? The proposal

For main OS paths issue

I suggest

  • We move every shared setting we want for quarto-cli in .vscode/settings.json so that it applies correctly for for windows user and avoid duplicates
  • to leave in quarto-cli.code-workspace only the settings that can't be in project settings (it seems some are like files.associations) and the Unix path.

This allows Windows dev (aka me 😅) to create a workspace .code-workspace file to have the windows path for the settings.

Other changes

We use different languages, and different tooling for Quarto cli. I figured it would be worth using the same VSCODE extensions, and settings with different language. So I added Python and Julia as settings, and some extensions as recommended like pyling for python linting, and black for formatters.

This is for discussion, and I'll comment inline about some settings choice.

For the deno.importMap setting

I don't know which one of the dev_import_map.json or import_map.json should be used. The currently the latter is the one used, I supposed this is ok and we can remove the one from quarto-cli.code-workspace. I'll add a note inline to discuss.

@cscheid @gordonwoodhull this is really for discussion. We can also discuss this live at our next meetting. No hurry, I just wanted to take notes on my findings as I needed to tweak all this to fix the deno settings.

@cderv cderv force-pushed the update-settings branch from 60b4672 to 74842a8 Compare May 31, 2024 10:43
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Among all our language, we usually set editor.formatOnSave: false, also because we opt in to true as a default (see lower in config file) https://github.com/quarto-dev/quarto-cli/pull/9817/files#diff-a5de3e5871ffcc383a2294845bd3df25d3eeff6c29ad46e3a396577c413bf357R30

This means our file are never auto formatted for some languages, but this can be trigger using Command pallet in VSCODE. For all others, it will be formatted using prettier.

I believe we did opt-out because prettier does not support everything, and some files where not style at first. Meaning if we set to true now, we will have a lot of style change, but better future I would say :)

If we want to enforce the same formatting for every files, and every language, we would activate this settings. Probably lots of style commits to do (in one big PR or over time), but this would ensure same style everywhere for everyone.

Currently, I left the config to false. This includes Lua. Currently, I don't think we use automatically the formatting feature from the Lua extension we use.

"editor.formatOnSave": false,
"editor.formatOnType": false,
"editor.wordBasedSuggestions": "off",
"editor.defaultFormatter": "ms-python.black-formatter"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found we could use Black formatter for Python from VSCODE doc: https://code.visualstudio.com/docs/python/formatting#_choose-a-formatter

The extension is this one:
https://marketplace.visualstudio.com/items?itemName=ms-python.black-formatter

Again if we do this, next time we'll update some .py file, there will be some style change. So if we choose to do this, I can't make a PR only with style change to anticipate

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems Ruff could be a good option too: https://marketplace.visualstudio.com/items?itemName=charliermarsh.ruff

Community based, but it seems a wide used formatter now (see #python discussion on slack)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might also want to add/use isort to organise Python imports which linter such as PyLint will flag:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, when working with Python, I am using: Pylance, isort, and Black Formatter.

"editor.defaultFormatter": "esbenp.prettier-vscode",
"editor.formatOnSave": true,
"editor.tabSize": 2,
"deno.enable": true,
"deno.lint": true,
"deno.unstable": true,
"deno.importMap": "./src/import_map.json"
"deno.importMap": "./src/import_map.json",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the settings we use, but it is overwriting the other one.

Is the value ok ? I believe so.

.vscode/settings.json Show resolved Hide resolved
@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label May 31, 2024
@mcanouil
Copy link
Collaborator

mcanouil commented Jun 14, 2024

FYI, I am using some other extensions to check the content of various files such as YAML and Markdown:

On the repository side, we use Super Linter and CodeSpell actions on PRs:

@cscheid cscheid added this to the v1.6 milestone Jun 17, 2024
@cderv cderv force-pushed the update-settings branch from f6e9bc5 to d9e4ed5 Compare July 25, 2024 10:44
@cscheid cscheid modified the milestones: v1.6, v1.7 Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion Issues that require a team-wide discussion before proceeding further
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants