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

Add .vscode/settings.json #9295

Merged
merged 5 commits into from
Jan 30, 2024
Merged

Add .vscode/settings.json #9295

merged 5 commits into from
Jan 30, 2024

Conversation

nwalters512
Copy link
Contributor

This PR adds an opinionated .vscode/settings.json file. Committing this to the repository is by no means optimal, as it makes doing local customization of this config more difficult. However, we feel the benefits of getting a functioning editor out-of-the-box outweigh the downsides. Hopefully the VSCode folks address microsoft/vscode#40233 so we can get the best of both worlds.

Comment on lines +6 to +9
// This is required to load extensions, which in turn is required to be able
// to format SQL files.
// https://github.com/prettier/prettier-vscode/issues/3235
"prettier.configPath": "./.prettierrc.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the key to getting SQL files formatting again; there's no need to install a separate extension.

Hopefully prettier/prettier-vscode#3235 gets addressed soon.

@jonatanschroeder
Copy link
Member

A few things in mine that are not in yours:

"isort.args": ["--profile", "black"],

not sure what the big difference would be here, but I found it relevant at some point.

"editor.codeActionsOnSave": {
    "source.fixAll.eslint": true
  },
  "eslint.validate": ["javascript", "typescript"],
  "typescript.updateImportsOnFileMove.enabled": "always",
  "javascript.updateImportsOnFileMove.enabled": "always"

self-descriptive.

"html.format.templating": true,

makes editing mustache easier.

@jonatanschroeder
Copy link
Member

I also have editor.formatOnType (format as soon as you hit enter on a line) for Python. editor.formatOnPaste could be useful too.

"editor.formatOnSave": true,
"editor.codeActionsOnSave": {
"source.organizeImports": "explicit"
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the configs I see add an argument to the isort command: https://stackoverflow.com/a/72667901

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Import sorting works fine for me without this. Doesn't isort pick up any needed config from pyproject.toml?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There are only some cases where black and isort might differ in behavior, but I had the same thought, it might just pick up the config from pyproject.

@nwalters512
Copy link
Contributor Author

@jonatanschroeder to pick those apart:

"editor.codeActionsOnSave": {
    "source.fixAll.eslint": true
},

This is covered in the JS/TS language sections already.

"eslint.validate": ["javascript", "typescript"],

https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint says "This is an old legacy setting and should in normal cases not be necessary anymore". Is there a particular reason you feel this is necessary?

"typescript.updateImportsOnFileMove.enabled": "always",
"javascript.updateImportsOnFileMove.enabled": "always"

IMO these are personal preferences, not the foundations of a useful editor experience.

"html.format.templating": true,

I enabled this but couldn't perceive any differences when editing an HTML files. Can you tell me what I should be looking for here?

I also have editor.formatOnType (format as soon as you hit enter on a line) for Python. editor.formatOnPaste could be useful too.

Again, I feel these are personal preferences, unless you have an argument for why they should be set by default.

@jonatanschroeder
Copy link
Member

"eslint.validate": ["javascript", "typescript"],

https://marketplace.visualstudio.com/items?itemName=dbaeumer.vscode-eslint says "This is an old legacy setting and should in normal cases not be necessary anymore". Is there a particular reason you feel this is necessary?

No reason. I saw them online combined with the eslint fix setting and added them together, so I thought they were both needed, glad to see that's not the case.

"typescript.updateImportsOnFileMove.enabled": "always",
"javascript.updateImportsOnFileMove.enabled": "always"

IMO these are personal preferences, not the foundations of a useful editor experience.

I don't have a strong opinion on these are they only affect cases where we rename or move files, so I'm ok with your assessment.

"html.format.templating": true,

I enabled this but couldn't perceive any differences when editing an HTML files. Can you tell me what I should be looking for here?

Indeed, this typically affects changing question.html files in my courses, but exampleCourse/testCourse are ignored by prettier in PL, so they won't affect anything here. Feel free to skip.

I also have editor.formatOnType (format as soon as you hit enter on a line) for Python. editor.formatOnPaste could be useful too.

Again, I feel these are personal preferences, unless you have an argument for why they should be set by default.

I'm ok with that assessment.

.vscode/settings.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
Co-authored-by: Jonatan Schroeder <jonatan@yorku.ca>
Copy link
Collaborator

@eliotwrobson eliotwrobson left a comment

Choose a reason for hiding this comment

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

LGTM

@nwalters512 nwalters512 added this pull request to the merge queue Jan 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2024
@nwalters512 nwalters512 added this pull request to the merge queue Jan 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 29, 2024
@nwalters512 nwalters512 added this pull request to the merge queue Jan 30, 2024
Merged via the queue into master with commit e69a116 Jan 30, 2024
6 checks passed
@nwalters512 nwalters512 deleted the add-vscode-settings branch January 30, 2024 00:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants