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

Migrate build to version catalogs #11684

Merged
merged 1 commit into from
Nov 25, 2024
Merged

Conversation

JL0000
Copy link

@JL0000 JL0000 commented Nov 8, 2024

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Moved project dependencies from build files to a version catalog file.

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@github-actions github-actions bot added the size/large PRs with less than 750 changed lines label Nov 8, 2024
@opusforlife2 opusforlife2 added codequality Improvements to the codebase to improve the code quality rewrite Issues and PRs related to rewrite labels Nov 8, 2024
@TobiGr TobiGr mentioned this pull request Nov 9, 2024
@Stypox
Copy link
Member

Stypox commented Nov 10, 2024

Thanks for the PR! This should go to the refactor branch though, as the dev branch is kind of in maintenance mode. You can change the base branch by clicking "Edit" next to the PR title.

@JL0000 JL0000 changed the base branch from dev to refactor November 10, 2024 15:05
@github-actions github-actions bot added size/giant PRs with more than 750 changed lines and removed size/large PRs with less than 750 changed lines labels Nov 10, 2024
@Profpatsch
Copy link
Contributor

Okay, so I checked this out, squashed the last 3 commits into one, and I have to say I’m not particularly in favor of this.

Before, we had a pretty simple file with dependencies and their version numbers, now we have yet another toml file, and that file has yet another indirection, so we are introducing two indirections here for very little gain.

I’m in favor of closing this PR and the referenced issue as WONTDO

@Profpatsch
Copy link
Contributor

I’d be okay with merging the change @JL0000 if you can clean up the history, squash everything into one commit and make it compatible with the current refactor branch.

@snaik20
Copy link
Contributor

snaik20 commented Nov 19, 2024

@JL0000 It seems like the PR contains a lot of unrelated commits, likely due to updating the base branch on GitHub. To resolve this, you can rebase your branch locally onto the latest refactor branch and then push the changes again.

@github-actions github-actions bot added size/large PRs with less than 750 changed lines and removed size/giant PRs with more than 750 changed lines labels Nov 22, 2024
@snaik20 snaik20 self-requested a review November 23, 2024 04:14
@snaik20
Copy link
Contributor

snaik20 commented Nov 23, 2024

@JL0000 Thanks for updating. I will review the changes today.

Copy link
Contributor

@snaik20 snaik20 left a comment

Choose a reason for hiding this comment

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

Thank you, @JL0000, for implementing the changes. I have updated the naming for a few libraries, and the changes look good to me.

One suggestion: I wonder if we could implement a system to enforce or lint this file to ensure the list remains sorted. If this proves too complex, we could address it in a separate ticket.

@Stypox Stypox merged commit e49156f into TeamNewPipe:refactor Nov 25, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codequality Improvements to the codebase to improve the code quality rewrite Issues and PRs related to rewrite size/large PRs with less than 750 changed lines
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Migrate project dependencies to using Version Catalogs
5 participants