-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat: add more default settings for VSCode #388
feat: add more default settings for VSCode #388
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thank you @thbeu. Also notice that I had added these two git hooks to my PR #386:
python-package-template/.pre-commit-config.yaml
Lines 120 to 122 in 3d7c80e
- id: check-json | |
- id: pretty-format-json | |
args: [--autofix] |
Looks like your git hooks didn’t run locally on commit?
Please note that I just closed my own PR #386 and changed the base of your, this PR to our staging
branch. We can use this PR and merge it straight into our mainstream.
One more question below.
Both check-json and pretty-format-json fail on settings.json and extensions.json where I added line comments. Not sure what to do? |
To be honest, I’m not certain either. I’m a little surprised that VSCode produces incorrect JSON (i.e. JavaScript style comments inside JSON) and the git hooks have a different opinion wrt. formatting. I’m a bit worried that forcing the git hooks over the VSCode generated files will continue to be headache, because every time a user changes the VSCode settings the hooks will complain. We could consider using exclude in the configuration, for example here python-package-template/.pre-commit-config.yaml Lines 1 to 5 in 7bf474b
we’d add exclude: ^.vscode/ That way VSCode can just own these files without the git hooks changing & checking them. |
I already played with that exclude setting but did not get it running:
|
Yup that’s expected output from this hook
Because we’ve excluded all JSON files using a global |
Argh, there is an EOL issue now in .pre-commit-config.yaml. It really would be helpful to have a
|
Probably caused by this hook: python-package-template/.pre-commit-config.yaml Lines 110 to 111 in 7bf474b
Oh sure, I’ll take a look, thanks! (Opened issue #389.) |
And removed again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the original VSCode files, yes?
What do you mean by original? EOL of the three JSON files is mixed, it seems. |
I mean that if a user opens the project using VSCode then they’ll work? If a user changes their settings and saves then it won’t impact any other aspect of the repo considering we’ve excluded the entire Let’s wait what @behnazh says but I’m ok with this change 👍🏼 |
Yes, they will work on Linux provided the venv is correctly setup. For Windows the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you've removed the exclude: ^.vscode/
from .pre-commit-config.yaml
?
.pre-commit-config.yaml is no longer touched by this PR. |
Because the VSCode configuration JSON file contains comments, our pre-commit hooks should fail. Did it pass for you? It would be good to exclude the |
The current However, if a user would add e.g. the check-json hook then yes, that hook would fail on the VSCode files (which aren’t actually valid JSON). So we should probably add exclude: ^.vscode/ back into |
Oops, my bad. I thought we already had added a check.
👍 |
@thbeu does exclude: ^.vscode/ back to the top of the |
Yes, that works, see 676ff34. |
@thbeu feel free to Squash and merge your PR now 👍🏼 |
Only contributors /w write access can do so. |
Good, you’re still the author of the merged commit 10154b3 — I wanted to ensure that you get the fame and blame for this change 😉 |
When will it go from jenstroeger:staging to main? |
We’ve not planned a new release yet. Do you have a desired date or a deadline? |
No, just being curious. |
This is for #386.