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 Makefile and .json to editorconfig #30324

Closed
wants to merge 2 commits into from
Closed

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jan 1, 2022

There are quite a lot of json files that were missing from this definition.
I've also added all Makefiles to the spec, because it is important to use tabs when editing them.

Docs: https://editorconfig.org/
Refs #27638
Refs https://bugs.python.org/issue44854

CC @ambv

Copy link
Member

@merwok merwok left a comment

Choose a reason for hiding this comment

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

I would decline this PR, I don’t see that it solves a problem.

[Makefile{,.pre.in,.pre}]
indent_size = 4
indent_style = tab
trim_trailing_whitespace = true
Copy link
Member

Choose a reason for hiding this comment

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

I would question an editor that does not properly handle makefiles!

Copy link
Member Author

Choose a reason for hiding this comment

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

That's something we cannot control 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Sure, this is harmless to have, why not add it.

trim_trailing_whitespace = true
insert_final_newline = true
indent_style = space

[*.{py,c,cpp,h}]
[*.{py,c,cpp,h,json}]
indent_size = 4
Copy link
Member

Choose a reason for hiding this comment

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

Formatting JSON files does not seem necessary for this repo. I have only found a few, and they seem to be imported from external sources or maintained without issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have 11 json files in the repo: https://github.com/python/cpython/search?q=language%3Ajson

I don't think json support is any different from yml, which we already have.

Copy link
Member

@merwok merwok Jan 2, 2022

Choose a reason for hiding this comment

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

I know, that’s what I called a few, and you’re ignoring my second note that they are mostly copied from external sources.

I am -1 on this change. It does not solve any problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, not meant to ignore your words at all! 🙂
It was more like a personal research, because I know that are not-so-many json files, but I was not sure about the exact amount.

I also agree that this is not a real "problem" at all, no one was ever hurt by the wrong number of spaces in a json file (at least I really hope so!).

But, the initial reason I've created this PR was this file in its current form: https://github.com/python/cpython/blob/main/.github/problem-matchers/msvc.json

I was reading through it and asked myself: why does it have mixed formatting? What is the rule? I looked at .editorconfig, but didn't find any. And since I was also fixing .editorconfig files in other python/* repositories, I've decided to create a PR that can potentially solve this tiny problem in the future.

But, I don't have strong feelings about a so-little-change, so I / you / anyone-else can totally close this PR if needed 👍

Thanks a lot for your feedback and review, TIL about tabstop!

Copy link
Member

Choose a reason for hiding this comment

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

With an old, stable codebase like CPython, the team prefers to solve real problems (reported by end users or core developers) and avoid cosmetic changes that don’t provide a concrete improvement, may introduce errors and muddle history (git log is not very good, not to mention github history that doesn’t even follow renames). Consistency in itself and for itself is not a goal! Thanks for your efforts, they will be very appreciated for other kinds of PRs!

Co-authored-by: Éric <merwok@netwok.org>
@merwok
Copy link
Member

merwok commented Jan 2, 2022

The changes for JSON are not wanted, and the remaining changes for makefiles seem too small to warrant a commit.
If people do use editors that recognize editorconfig settings but do not handle makefile(.pre)(.in) properly, then a bug report could be opened and this would be reconsidered.

@merwok merwok closed this Jan 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants