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

Update PR workflow / workspace #1053

Merged
merged 33 commits into from
Mar 6, 2024

Conversation

Archmonger
Copy link
Contributor

@Archmonger Archmonger commented Jun 15, 2023

This contribution is made under the MIT license.

Issues

N/A

Summary

  • Add VS Code extension suggestions
  • Reorganize pull request template

Checklist

  • Tests have been included for all bug fixes or added functionality.
  • The changelog.rst has been updated with any significant changes.

@Archmonger
Copy link
Contributor Author

@rmorshea Can you send me your ruff vscode config? I can't seem to get it to not auto format everything in an extremely annoying way in my environment

.github/pull_request_template.md Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@rmorshea
Copy link
Collaborator

rmorshea commented Jun 15, 2023

will send the ruff config tomorrow

@rmorshea
Copy link
Collaborator

It seems like there's not a whole lot special about my config. I just have the charliemarsh.ruff extension installed and the following:

project:

{
  "python.formatting.provider": "none",
  "[python]": {
    "editor.defaultFormatter": "ms-python.black-formatter"
  }
}

user:

  "python.languageServer": "Pylance",
  "python.analysis.autoImportCompletions": true,
  "python.formatting.provider": "black",
  "[python]": {
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.fixAll": true
    },
    "editor.formatOnType": true
  },

It's possible that our Ruff config in pyproject.toml could use some refinement. For example, Ruff aggressively removes unused code which, if you save while you're working on something, can be annoying.

@Archmonger Archmonger marked this pull request as ready for review June 16, 2023 11:19
@Archmonger Archmonger requested a review from rmorshea June 16, 2023 11:20
.vscode/settings.json Outdated Show resolved Hide resolved
rmorshea
rmorshea previously approved these changes Jul 4, 2023
@Archmonger
Copy link
Contributor Author

Tests are borked again, preventing merge.

@Archmonger
Copy link
Contributor Author

I can't self-merge on this repo so I will need another approval.

.vscode/settings.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
{
"editor.detectIndentation": false,
"editor.formatOnSave": true,
"python.linting.enabled": true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting python.languageServer and python.liniting.enabled allows for VS Code to warn the user if it's not installed.

@@ -0,0 +1,35 @@
{
"editor.detectIndentation": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why we need to configure this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without customizing this, sometimes VS Code will try to use tabs instead of spaces depending on the file type.

"editor.formatOnSave": true,
"python.linting.enabled": true,
"python.linting.mypyEnabled": true,
"python.analysis.typeCheckingMode": "off",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is already the default except on VSCode insiders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we keep it off in anticipation of the insiders build coming to main?

Comment on lines 10 to 13
"prettier.tabWidth": 4,
"prettier.useTabs": true,
"prettier.endOfLine": "auto",
"prettier.proseWrap": "never",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I use all the defaults for Prettier. I also think it will auto detect project configuration by looking for one of the places it might be set: https://prettier.io/docs/en/configuration.html

Copy link
Contributor Author

@Archmonger Archmonger Jul 18, 2023

Choose a reason for hiding this comment

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

Without proseWrap and endOfLine, prettier will auto-format jinja templates in a way that breaks them.

.vscode/settings.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
@Archmonger
Copy link
Contributor Author

Let me know if you agree/disagree with the open comments.

rmorshea
rmorshea previously approved these changes Jul 27, 2023
@Archmonger
Copy link
Contributor Author

I can't merge due to lint errors.

@Archmonger
Copy link
Contributor Author

I'm realizing it's going to get annoying to maintain a vscode config file. Microsoft tends to deprecate/delete settings very frequently, and I don't want us to deal with that.

I've changed this PR to only have suggested workspace extensions.

@Archmonger Archmonger changed the title Add VS Code config Update PR workflow / workspace Jan 11, 2024
@Archmonger Archmonger merged commit 618e579 into reactive-python:main Mar 6, 2024
17 checks passed
@Archmonger Archmonger deleted the workspace-config branch March 6, 2024 02:50
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.

2 participants