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 defaults #1806

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add vscode defaults #1806

wants to merge 1 commit into from

Conversation

neheb
Copy link
Collaborator

@neheb neheb commented Dec 2, 2024

It seems people are still removing newlines from files. Try to prevent.

It seems people are still removing newlines from files. Try to prevent.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Multiplication of editor-specific configuration files for this kind of thing are an absolute antipattern. Let's not do this.

@eli-schwartz
Copy link
Member

Please strongly urge vscode users that in order to respect the project coding standards they must install https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig

@dcbaker
Copy link
Member

dcbaker commented Dec 2, 2024

As a vscode user I highly suggest editorconfig as well.

I wonder if it would be better to add a linting checker to the MR that will fail if the final newline is removed?

@trim21
Copy link

trim21 commented Dec 3, 2024

It is possible to use vscode extension suggestion file to recommand vscode users to use EditorConfig, {"recommendations": ["EditorConfig.EditorConfig"]} in ./.vscode/extensions.json and it doesn't affect user in other way or interactive users' current installed plugin.

And there are existing tools to apply editor config check on files: https://github.com/editorconfig-checker/editorconfig-checker

@aaalloc
Copy link

aaalloc commented Dec 3, 2024

You could also use something similar to https://pre-commit.com/ and do a a pre github action for checking result of pre-commit before launching other actions for building

something similar to https://github.com/openslide/openslide-python/blob/602ddd8642e30d43cf5faaf27ee639b7ff1eb911/.github/workflows/python.yml#L18

https://github.com/openslide/openslide-python/blob/main/.pre-commit-config.yaml

@eli-schwartz
Copy link
Member

There's nothing "also" about that. pre-commit.com is just an inferior hijack of the concept of git pre-commit hooks, using an unreasonably restrictive and insecure dev workflow. The actual thing you end up running inside pre-commit.com would still be the same lint tool you would otherwise use directly in GitHub Actions.

So that's really just a reiteration of

I wonder if it would be better to add a linting checker to the MR that will fail if the final newline is removed?

I think adding a linting checker in CI is the correct way to handle the concern that people aren't always enabling the editorconfig extension in their text editors. Although I'm opposed to using pre-commit.com as the entrypoint loader for such a CI, for several reasons.

@dcbaker
Copy link
Member

dcbaker commented Dec 4, 2024

On the meson files themselves, now that we have meson format, it probably makes sense to have a meson.format and apply that to all of the incoming meson.build files.

Using at least flake8 or ruff for linting incoming python makes sense to me as well (but not formatting, ruff replicates the worst parts of black).

I played a little with editorconfig-check but couldn't get it to apply only to text and json files, which I what I wanted, and it has issues with meson and python files, because sometimes continuing indents don't line up the way it wants, consider:

def my_function(self,
                           *arguments: Union[str, bytes, pathlib.Path[str], pathlib.Path[bytes],
                           **kwarguments: Union[int, float, decimal.Decimal],
                           ) -> dict[str, Optional[Tuple[pathlib.Path[str], decimal.Decimal]]:
    ...

editorconfig-check will complain that those aren't 4 space indented, even though the trailing indent is an acceptible python style and flake8 will be happy with it.

@dcbaker
Copy link
Member

dcbaker commented Dec 4, 2024

I'm also not opposed to adding the vscode suggested extensions thing, since that isn't duplicating information with .editorconfig, and might help clean some of this up sooner, but I'm also not going to fight for it if someone else is strongly opposed

@dcbaker
Copy link
Member

dcbaker commented Dec 4, 2024

I started looking at adding some CI support for this issue: #1807

@eli-schwartz
Copy link
Member

I am strongly opposed, because it increases root clutter in the online file view without IMHO providing value in exchange. It's the exact problem that EditorConfig was created to solve, so it feels disappointing to me that people's reaction is "ok then let's add text editor specific config files that say to please respect the first file". If we just want a list of "recommendations", we already expect people to read the README.md, and neither approach actually enforces anything. So we can just recommend it in the contributing instructions, and that combined with linting in .github/workflows/ should I think cover all needs.

editorconfig-check will complain that those aren't 4 space indented, even though the trailing indent is an acceptible python style and flake8 will be happy with it.

Interesting, sounds like an upstream bug in that tool then. Pity.

In theory it's possible to tell .editorconfig to not specify indent size, do we actually need that setting? Usually python-aware text editors will already set the indent size to the same thing we are enforcing.

@dcbaker
Copy link
Member

dcbaker commented Dec 4, 2024

In theory it's possible to tell .editorconfig to not specify indent size, do we actually need that setting? Usually python-aware text editors will already set the indent size to the same thing we are enforcing.

Yeah, we really do. Because a lot of people set their editors to universally do 8 space tabs, or 8 spaces regardless of what language they're working in. Especially C programmers.

@eli-schwartz
Copy link
Member

Wow... :(

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.

5 participants