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

editorconfig: Remove unnecessary rules #433

Closed

Conversation

softworkz
Copy link
Collaborator

None of these rules are needed for Visual Studio and these also do not stem from any defaults/templates of VS.

None of these rules are needed for Visual Studio and these also do not stem from any defaults/templates of VS.
@softworkz
Copy link
Collaborator Author

This might also solve some of the problems which @JunielKatarn is having.

It should further avoid unrelated whitespace changes and end-of-file changes in future PRs.

Copy link
Member

@lukasf lukasf left a comment

Choose a reason for hiding this comment

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

Right. The crlf there, in combination with not using autocrlf setting is probably causing part of the problems @JunielKatarn has.

@softworkz
Copy link
Collaborator Author

Right. The crlf there, in combination with not using autocrlf setting is probably causing part of the problems @JunielKatarn has.

He's working on Linux and using a different IDE (Linux because he said it and different IDE because there's no VS for Linux).
I'm very thankful that you accepted some changes that helped me for my use cases and I'm all into helping him to get his scenario working as well. I never meant to be rejective, it's just that I do have that shitload of experience that I know upfront from many commits, which problems these will (or may) cause in the future.
It would be easier if he would be a bit more open about his setup, but based on all that is known by now, this should significantly improve his use case and I hope this PR also makes it clear that I want to help rather than block or reject. 😄

@brabebhin
Copy link
Collaborator

If i had to guess he is using Rider.

@softworkz
Copy link
Collaborator Author

Since he appears to be affiliated with MS in some form, a very steep speculation would be a secret prototype of a "VS for Linux". It's the most requested topic of all times on the MS Dev Community site (officially rejected though). That would explain why he doesn't tell (not allowed). ;-)

@brabebhin
Copy link
Collaborator

But Microsoft has its own version of this library. I don't think he'd bother with us in a work context.

@softworkz
Copy link
Collaborator Author

But Microsoft has its own version of this library. I don't think he'd bother with us in a work context.

I don't mean he works on it, it would be about dog-feeding (letting employees from different projects use it for testing).

@JunielKatarn
Copy link
Collaborator

Hi!

I'm currently out of town and away from keyboard, so haven't had time to review feedback on my PR nor look into this discussion, but notifications show I've been alluded a few times so I'll post some clarifications in my next reply...

@softworkz
Copy link
Collaborator Author

In case there's something you wouldn't like to see written here, just let me known and I'll delete it.

@JunielKatarn
Copy link
Collaborator

Let me start saying I do have issues with this PR.

Unless we don't want to use an editor helper tool (i.e. EditorConfig) at all and let whatever formatting issues and potential conflicts happen, we shouldn't drop the whitespace trimming rule.
I won't press hard on this, but don't understand the value of removing the automated editor formatting.

Now, into the mentions:

Right. The crlf there, in combination with not using autocrlf setting is probably causing part of the problems @JunielKatarn has.

Partially. It all comes down to that line endings we want for what files.
Issues will happen even if I set AUTOCRLF on Windows (which I have for this project).
AUTOCRLF forces CRLF even for files that are meant to be LF. Won't get into this topic here. I will open a discussion next week.

Right. The crlf there, in combination with not using autocrlf setting is probably causing part of the problems @JunielKatarn has.

Not exactly. It's only disabling AUTCRLF that induced the huge diffs. This was intentional (I intended to make VS-specific project files CRLF in the repository as opposed to LF). Again, won't go further into it here.

He's working on Linux and using a different IDE (Linux because he said it and different IDE because there's no VS for Linux).

macOS to be specific, but the same semantics apply to Linux.
Not a different IDE, I actually use VS Code for everything that does not require compiling and running the project (then I open Visual Studio on Windows). I have synchronized settings so the behavior is identical in all OSes.

It would be easier if he would be a bit more open about his setup

Happy to share any further details.
If it helps, generally speaking:

  • VS Code for almost all text editing purposes, with EditorConfig enabled. I continuously switch between major operating systems: Windows, Linux, macOS. The IDE's behavior is consistent and identical.
  • Visual Studio 2022 (always updated to the lates release). No special add-ins or settings.
  • Git command line (default settings except for AUTOCRLF set to false on Windows; re-enabled exclusively for this project's local repositories). Installed from the "standard" sources (apt, Xcode, Visual Studio installer)

@JunielKatarn
Copy link
Collaborator

Since he appears to be affiliated with MS in some form

I am indeed affiliated with MS in some form :)
However, my involvement in this project is 100% out of personal interest and using only personal resources.

a very steep speculation would be a secret prototype of a "VS for Linux".

I wish!
Sadly, not only not the case, but there was a Visual Studio for Mac that is getting retired literally today.

It's the most requested topic of all times on the MS Dev Community site (officially rejected though). That would explain why he doesn't tell (not allowed). ;-)

Sorry if I missed any direct questions on this regard. I was very sporadically available this week.
Based on my previous answer, I would guess there are just no plans to develop other ports for Visual Studio.
My guess is VS Code is the intended to be a cross-platform IDE (it does support .NET, CMake and actual C++ on all platforms).

# Xml project files
[*.{config,csproj,njsproj,targets,vcxitems,vcxproj,vcxproj.filters}]
indent_size = 2
end_of_line = crlf
insert_final_newline = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will cause EditorConfig-enabled editors to insert a newline, where Visual Studio trims such newlines when modifying those types of files.

It will cause on and off final newline diffs depending on what tool was used to edit such files.

I strongly recommend to keep this one.
To reiterate, it's not about the VS templates only, but the IDE's editing behavior.

Copy link
Collaborator Author

@softworkz softworkz Aug 31, 2024

Choose a reason for hiding this comment

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

his will cause EditorConfig-enabled editors to insert a newline

No, this is not true. If no rule exists for this in editorconfig, then it's the editor's default behavior coming into play. For most editors it's based on auto-detection or defaults of the editor. The editorconfig spec defines no default for this type of rule, so having no rule at all is the most compatible setting.

where Visual Studio trims such newlines when modifying those types of files.

VS doesn't trim. It handles mixed line endings transparently without modifying. Only when adding new lines it will choose a line ending style (from auto-detection or VS options settings).

It will cause on and off final newline diffs depending on what tool was used to edit such files.

No. If this rule isn't present, an editor normally doesn't make any change to it. (VS doesn't)

Copy link
Member

Choose a reason for hiding this comment

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

I think this remark is not about EOL, but about insert_final_newline (newline at end of file). It might be good to keep it as @JunielKatarn suggested.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the remark is about newlines at the end of files, then the statement is still incorrect. Visual Studio does not trim any CR or LF at the end of files when editing.

VS leaves everything as is and that's the behavior that we need. On the contrary, the existence of those rules has caused the PR's from @JunielKatarn to include such changes to the end of files which we neither want nor need (assuming that the current state is how we want everything to be).

It's generally important to understand that removing such rules doesn't mean that the opposite would happen. Removing rather means that the editor won't make changes in that regard.

Also important: Visual Studio doesn't need any editorconfig rules at all. It works totally fine without any editorconfig in place.
It's also very unusual to have those EOL and EOF rules when using VS. Never seen that before, it's in none of the templates, nobody does that because there's no need for this in case of VS. It does these things right just out of the box.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say we should leave insert_final_newline = false in. These are specific VS formats. VS never adds final new lines and removes them when found. It is safe and reasonable to leave this setting in the editorconfig, so other code editors won't accidentially insert final new lines when it is their default.

@softworkz
Copy link
Collaborator Author

softworkz commented Aug 31, 2024

Unless we don't want to use an editor helper tool (i.e. EditorConfig) at all and let whatever formatting issues and potential conflicts happen, we shouldn't drop the whitespace trimming rule.
I won't press hard on this, but don't understand the value of removing the automated editor formatting.

The reason for removing it is specifically about your submissions which are containing those whitespace changes which don't belong into commits/PRs with functional changes because the whitespace changes are distracting from the actual changes (like I and @lukasf said in response to your PR).
Same goes for the line break at end of file rule.

Now, I don't know VSCode well enough regarding .editorconfig behavior, but on 2 or 3 occasions, you mentioned that your editor would apply editorconfig rules to the whole file when saving a file. And that's a real problem, because it means that you aren't easily able to work in a way that you can commit functional changes which do not include editorconfig-based changes to lines which don't carry any functional change.

Visual Studio is very different here: It applies editorconfig rules only to the lines which you are actually editing and doesn't apply those rules to any other line. And it applies those rules "live" while editing, not on save.
If everybody would be using Visual Studio only, then we could easily keep those rules. But when it causes you to submit PRs which include editorconfig changes to all lines in a file, then that's a problem and it's better to not have those rules at all than having to discuss this it each time.

Now for the line endings: I think we are all good about the autocrlf setting at the Git side. You have it false on Linux and true on Windows (at least for this repo) - so that's already perfectly in line with what we all are doing.

The second side regarding line endings is the end_of_line setting in editorconfig. You had added the editorconfig file in a PR like 2 years ago, setting it to LF for all files. Two weeks later, @lukasf had changed it to CRLF presumably because he experienced those kind of problems that I've been talking about all the time (if you haven't done yet, I'd kindly ask you to watch my 2 part video response on this subject).
The setting of end_of_line=CRLF (applied to all files *.*) doesn't hurt on Windows, but isn't it that what's troubling you?
Your argument to set end_of_line=LF for editorconfig files was that the default for editorconfig would be CRLF, but that's not a default per spec, that's just the line above (for *.*).
This PR removes that line and all other end_of_line rules, so that should solve a large part of your problems.
If there are specific files which you need to be CRLF (even on MacOS), then we can add such specific rules to editorconfig, because setting end_of_line to CRLF doesn't hurt in Windows, but setting it to LF does (see my videos).

@softworkz
Copy link
Collaborator Author

Since he appears to be affiliated with MS in some form

I am indeed affiliated with MS in some form :) However, my involvement in this project is 100% out of personal interest and using only personal resources.

macOS to be specific, but the same semantics apply to Linux.
Not a different IDE, I actually use VS Code for everything that does not require compiling

Thanks a lot for sharing all this info. This allows to get a better understanding of each others workflow and setup and makes it so much easier to find a good solution which works well for everybody! I'm sure we'll be able to figure something out.

@softworkz
Copy link
Collaborator Author

@JunielKatarn - from my research, it appears that VSCode itself does not apply those rules like whitespace trim to all lines when saving. It only applied this to edited lines (just like the real Visual Studio). Can you confirm this?

I further read that there's a "Editorconfig for VS Code" extension which causes VS Code to apply rules to all lines when saving - do you have that extension installed?

@lukasf lukasf deleted the branch ffmpeginteropx:winui-build September 7, 2024 10:19
@lukasf lukasf closed this Sep 7, 2024
@lukasf
Copy link
Member

lukasf commented Sep 7, 2024

Oh this one got automatically closed as I merged+deleted the winui branch. Please Create a new PR against the master branch.

@softworkz
Copy link
Collaborator Author

Oh this one got automatically closed as I merged+deleted the winui branch. Please Create a new PR against the master branch.

Done: #436

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants