Skip to content

Conversation

@Nirmal4G
Copy link
Contributor

@Nirmal4G Nirmal4G commented Jan 21, 2022

Contributes to #4779 and #6645

Context

#7169 (comment)

Just as we removed xmlns from all XML build files, with this patch, we also remove <?xml?> tag from all XML-derived files in the repo. All these files are by default utf-8 and version 1.0 unless specified otherwise. So, it is safe to remove the meta tag.

Changes Made

  • Adjust new line between last two XML elements.
  • Remove XML meta tag (<?xml?>) from all XML-based files.
  • Remove trailing new-line from all XML-based files.
  • Remove trailing white-space from all XML-based files.

Testing

As long as CI succeeds! and we have some time to dogfood before shipping next release, I think!

Notes

Rebase merge if possible. Git introduced a new feature where a repo can ignore certain commits that'll mess up git blame. If this was to be merged as a merge commit and if that commit was ignored, it'll ignore non-whitespace changes too. So, Rebase merge if possible!

Adjust new line between last two XML elements.
Remove XML meta tag (`<?xml?>`) from all XML-based files.
Remove trailing new-line from all XML-based files.
Remove trailing white-space from all XML-based files.
@Nirmal4G Nirmal4G force-pushed the hotfix/sdk-prep/remove-xml-meta branch from 5978b3b to 1d173c0 Compare January 21, 2022 16:31
@Forgind
Copy link
Contributor

Forgind commented Jan 21, 2022

Your branch name says hotfix, but is this actually a hotfix for something serious? If so, what? It looked more like cleanup to me.

Rebase merge if possible. Git introduced a new feature where a repo can ignore certain commits that'll mess up git blame. If this was to be merged as a merge commit and if that commit was ignored, it'll ignore non-whitespace changes too. So, Rebase merge if possible!

We mostly squash things, though you have pretty clean commits for this PR.

@sharwell
Copy link
Contributor

Honestly, I prefer to keep this tag. 😬

@sharwell
Copy link
Contributor

Remove trailing new-line from all XML-based files.

I don't understand how people can tolerate this
image

@sharwell
Copy link
Contributor

Adjust new line between last two XML elements.

This change seemed to add a bunch of inconsistency. If the file has no other blank lines, I prefer to keep the end that way. The only case where I can see making this change is if the file already had a blank line after the opening element, in which case the blank line at the end matches.

@rainersigwald
Copy link
Member

When you proposed this before, we declined it. I don't think anything has changed based on the comments you linked. Why should we take this now?

@Nirmal4G
Copy link
Contributor Author

@Forgind

Your branch name says hotfix

I use Git Flow, I should setup some additional branch roots. Sorry for the confusion!

We mostly squash things, though you have pretty clean commits for this PR.

Thanks, and it's okay. Whatever works for the repo!

@Nirmal4G
Copy link
Contributor Author

@sharwell

I don't understand how people can tolerate this

Do you prefer having new-line at the end of file for structured (XML, C#, etc…) files?

This change seemed to add a bunch of inconsistency.

This is for consistency! 😅

if the file already had a blank line after the opening element, in which case the blank line at the end matches.

Exactly this!? I did check before; some files don't have matching new-lines. This patch has that applied.

@Nirmal4G
Copy link
Contributor Author

@rainersigwald

When you proposed this before, we declined it.

When? I did propose this in an issue but never really went along until I saw #7169. So, I thought, since, we just removed xmlns, this would be a nice time to remove <?xml?> tags too!?

@Nirmal4G Nirmal4G marked this pull request as draft January 21, 2022 17:25
@rainersigwald
Copy link
Member

#6645 was closed in #6645 (comment).

@rainersigwald
Copy link
Member

Do you prefer having new-line at the end of file for structured (XML, C#, etc…) files?

The almost universal UNIXy style preference, demonstrated by both core Git and GitHub's UI, is that all text files end with a newline.

@Nirmal4G
Copy link
Contributor Author

#6645 was closed in #6645 (comment).

I thought it was only for BOM removal in source files!? Do you not want all the other changes? I only did this because of #7169!

…almost universal UNIX-y style preference…all text files end with a new-line.

I don't prefer it for structured files where it can end with a tag or end-symbol since, hidden symbols could bleed into that new-line and could cause all sorts of problems! But I do prefer in regular text or semi-structured files like YAML, Markdown, INI-like files!

@sharwell
Copy link
Contributor

sharwell commented Jan 21, 2022

Do you prefer having new-line at the end of file for structured (XML, C#, etc…) files?

Yes. This seems to be the normal expectation for users as well (users seem split between either not caring, or caring and having a preference for the trailing newline, but the rare outlier is a user who prefers to remove it).

This is for consistency! 😅

It appears to have not achieved this goal. While two separate files may be consistent with each other, the more common review case is looking at just one file by itself, and in that context the blank line is not consistent.

@Nirmal4G
Copy link
Contributor Author

the more common review case is looking at just one file by itself, and in that context the blank line is not consistent.

This is my intention as well. Can you mark which files that are not consistent, I'll fix it!

@sharwell
Copy link
Contributor

Can you mark which files that are not consistent, I'll fix it!

I was not able to identify any file which was inconsistent or needed a fix prior to this PR.

@rainersigwald
Copy link
Member

I don't think this PR should be merged; it seems to be a contentious style change which isn't a good use of the team's time given the other work we could be pursuing.

@Nirmal4G
Copy link
Contributor Author

Maybe not now but sometime far in the future? 😅 So, keeping it in draft wouldn't hurt, right? 😉

@rainersigwald
Copy link
Member

I don't think leaving it open has much benefit, since there's no clear direction. I'd rather close it and not have any long-term draft PRs.

@Nirmal4G
Copy link
Contributor Author

@sharwell

Honestly, I prefer to keep this tag.

Some files don't have but some do. What can we do about this?

I was not able to identify any file which was inconsistent or needed a fix prior to this PR.

The files I touched did have a newline at the beginning but not at the end. I patched those. It seems I missed some files—especially the csproj ones. I'll fix those too.

@Forgind
Copy link
Contributor

Forgind commented Jan 22, 2022

I'm sorry @Nirmal4G; I'm normally the most in favor of refactoring PRs, but I have to side with rainersigwald on this one. A lot of the changes here will be automatically undone by editors as soon as the file is touched anyway, so that just feels like pointless churn. There are a few changes here I do like, but I don't think it's worth looking through carefully to find those.

@Nirmal4G Nirmal4G closed this Jan 23, 2022
@Nirmal4G Nirmal4G deleted the hotfix/sdk-prep/remove-xml-meta branch January 23, 2022 07:28
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.

4 participants