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

chore: switch to revanced library and bump patcher #1314

Merged
merged 8 commits into from
Oct 5, 2023

Conversation

Axelen123
Copy link
Member

No description provided.

@Axelen123 Axelen123 added the ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager label Sep 26, 2023
@Axelen123 Axelen123 marked this pull request as ready for review September 26, 2023 18:21
@Axelen123
Copy link
Member Author

Currently waiting on ReVanced/revanced-patches#2929

@oSumAtrIX
Copy link
Member

What is the reason for waiting? Compose is still a feature branch

Currently waiting on ReVanced/revanced-patches#2929

@Ushie
Copy link
Member

Ushie commented Sep 26, 2023

Not to break patching in the mean time for the devs

@Axelen123
Copy link
Member Author

Compose manager is not a feature

@oSumAtrIX
Copy link
Member

oSumAtrIX commented Sep 27, 2023

Currently it's not in main and dev branch, and is to be merged in dev as a feature branch. This PR has no reason to depend on patches. The development environment is to be responsible to handle the issue and mock the API

@Axelen123
Copy link
Member Author

Compose manager is a functional project just like flutter manager. Merging this would break it and there is no reason to do so until patches have updated. compose-dev not being merged to dev has nothing to do with this

@oSumAtrIX
Copy link
Member

Compose manager is a functional project just like flutter manager. Merging this would break it

No, it is in a development branch. It does not matter if it's functional or not, PRs like this should not be blocked and instead, the development environment as previously suggested should cover your issue.

compose-dev not being merged to dev has nothing to do with this

It does because compose-dev is considered a feature branch until merged to dev and then main. Until then the development environment is supposed to handle cases like pinning the patch version.

there is no reason to do

Development is being blocked unnecessarily and merge conflicts can arise if the PR is open long enough, which are both good reason to merge, regardless of the fact that the patches that manager has no relation with should not be a blocking reason to begin with in a developing environment.

@Axelen123
Copy link
Member Author

Compose manager does not belong here if it cannot operate as it used to before the migration.
Merge conflicts will be dealt with as they happen. Flutter manager does not bump patcher until patches support/require it and neither should compose manager.

@oSumAtrIX
Copy link
Member

Compose manager does not belong here if it cannot operate as it used to before the migration.

That does not change that it is not facing production and is in development. Again, the development environment has to cover your case, and not block this PR.

Flutter manager does not bump patcher until patches support/require it

Because it is in production. There is no development environment in production like there has to be for compose.

@Axelen123
Copy link
Member Author

Compose manager is very close to release. I do not want a delayed patches release to delay a compose manager release. This is why I want the compose-dev branch to be compatible with main patches.

@oSumAtrIX
Copy link
Member

Compose manager is very close to release.

This is fine, even if it were a day, the PR should not be blocked by patches.

I do not want a delayed patches release to delay a compose manager release.

But you are doing the opposite by blocking this PR, required for a release, due to patches release?

This is why I want the compose-dev branch to be compatible with main patches.

The compose dev branch will eventually be merged to the real dev branch and then to the main, at which point the patcher will have to be in sync for patches and manager. But currently you are delaying merging to compose-dev, far from production with reason that it would not be compatible with the currently released patches whereas:

the development environment as previously suggested should cover your issue.

@Axelen123
Copy link
Member Author

This PR is not required for releasing compose manager unless patches require it.
The dev branch of this repository also syncs its patcher version with the main branch of patches, so I am not sure why this was brought up. Manager is a user-oriented tool, so it makes sense to target the "production" version of patches.

@oSumAtrIX
Copy link
Member

The dev branch of this repository also syncs its patcher version with the main branch of patches, so I am not sure why this was brought up.

Yes because dev is gonna be merged to main, production, where the patcher version has to be synced when breaking changes are involved, whereas in this branch it does not as it is not gonna be merged to main after it reaches compose-dev. Once it is in dev, there we need to back off and sync.

Manager is a user-oriented tool, so it makes sense to target the "production" version of patches.

Only user-facing manager is main. And until it reaches main, the development environment has to catch situations like this one.

@Axelen123
Copy link
Member Author

Yes because dev is gonna be merged to main, production, where the patcher version has to be synced when breaking changes are involved, whereas in this branch it does not as it is not gonna be merged to main after it reaches compose-dev. Once it is in dev, there we need to back off and sync.

Syncing will take time if the other repositories suffer delays.

Only user-facing manager is main. And until it reaches main, the development environment has to catch situations like this one.

That does not change the nature of the project. Using newer versions of patcher makes sense for CLI because its more developer oriented, but manager is user-oriented.

@oSumAtrIX
Copy link
Member

That does not change the nature of the project. Using newer versions of patcher makes sense for CLI because its more developer oriented, but manager is user-oriented.

It does not face the user until its in main. Until its in main, the dev env has to be able to provide you with everything necessary to develop the app in its state.

@Axelen123
Copy link
Member Author

Axelen123 commented Oct 1, 2023

It does not face the user until its in main. Until its in main, the dev env has to be able to provide you with everything necessary to develop the app in its state.

Managers API client currently always chooses non-dev releases. The releases endpoint does have an option to select dev releases, but implementing that is not within the scope of this PR.

@oSumAtrIX
Copy link
Member

Managers API client currently always chooses non-dev releases. The releases endpoint does have an option to select dev releases, but implementing that is not within the scope of this PR.

Yes, this is why I am saying that the dev env has to cover this and since it is out of scope of this PR this PR should not be blocked just because of that.

@Axelen123
Copy link
Member Author

The dev env currently does not cover the situation because of what I mentioned, so the PR is still blocked.

@oSumAtrIX
Copy link
Member

In this case, if this PR is blocked by the dev env not supporting it, a PR should be made to allow that before merging this PR.

@Axelen123
Copy link
Member Author

Open an issue

@oSumAtrIX
Copy link
Member

Moving to #1326

@Axelen123 Axelen123 merged commit e232044 into compose-dev Oct 5, 2023
1 check passed
@Axelen123 Axelen123 deleted the compose/revanced-lib branch October 5, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ReVanced Manager Compose Regarding the Compose rewrite of ReVanced Manager
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants