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

Format all XML resources #4471

Merged
merged 2 commits into from
Oct 14, 2020
Merged

Format all XML resources #4471

merged 2 commits into from
Oct 14, 2020

Conversation

wb9688
Copy link
Contributor

@wb9688 wb9688 commented Oct 9, 2020

What is it?

  • Bug fix (user facing)
  • Feature (user facing)
  • Code base improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

Format all XML resources using Ctrl+Alt+L in Android Studio. I've also manually added newlines at the end of XML resources that currently didn't have a newline.

Fixes the following issue(s)

Seeing stuff like https://github.com/TeamNewPipe/NewPipe/pull/4453/files#diff-8093b831702ba14428d2d99c9edded66 in PRs, which makes it harder to review.

Agreement

@avently
Copy link
Contributor

avently commented Oct 9, 2020

Finally. THANK YOU.

@connectety
Copy link
Contributor

Nice work.
IMO this PR should add xml-linting to make sure the work will not be undone in like a year.
I mean this can always be checked in the review, but that will often mean an extra commit and might be just forgotten.

@wb9688
Copy link
Contributor Author

wb9688 commented Oct 10, 2020

@connectety: That was also what I was thinking, however I'm not aware of a way to run Android Studio's XML formatter from the command line.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Can we just assume Ctrl+Alt+L does his job correctly and merge directly? ;-)
345 files are too many to check ;-)

Anyway, I took a brief look and there does not seem to be any strange things. I'm not sure if we would need to do some deeper analysis or we can just trust AS. Maybe we can so something like this?

app/src/main/res/layout/player_notification.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/player_notification_expanded.xml Outdated Show resolved Hide resolved
@wb9688 wb9688 requested a review from Stypox October 11, 2020 09:38
@wb9688
Copy link
Contributor Author

wb9688 commented Oct 11, 2020

@Stypox: It's just some XML, I'd trust Android Studio for that. Also from taking a quick look at what it did (for the first about 100 files).

@wb9688
Copy link
Contributor Author

wb9688 commented Oct 11, 2020

Stypox
Stypox previously approved these changes Oct 11, 2020
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Ok, then @TobiGr merge

@TacoTheDank
Copy link
Member

You have no idea how happy this makes me.

@opusforlife2
Copy link
Collaborator

@TacoTheDank You do seem like the kind of person who likes a well-kept codebase. 🤭

@Stypox
Copy link
Member

Stypox commented Oct 14, 2020

For some reason the Synk vulnerability checks are failing, but I don't have the rights to access the results and find out why

@TobiGr
Copy link
Member

TobiGr commented Oct 14, 2020

snyk is also failing for other PRs. There are no dependency changes in this PR, so this should be good to merge.

@TobiGr TobiGr merged commit 613070d into TeamNewPipe:dev Oct 14, 2020
This was referenced Nov 10, 2020
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.

7 participants