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

Remove obsolete code #12955

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Remove obsolete code #12955

merged 2 commits into from
Jan 22, 2024

Conversation

TFleury
Copy link
Contributor

@TFleury TFleury commented Dec 15, 2022

Remove code marked obsolete for more than two years.

Copy link
Contributor

@Skrypt Skrypt left a comment

Choose a reason for hiding this comment

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

Need to see if we should keep them and remove them only in a major release.

@hishamco
Copy link
Member

Agree with @Skrypt to remove them on the next major release

@agriffard
Copy link
Member

agriffard commented May 6, 2023

Still to remove obsolete code.

@Piedone
Copy link
Member

Piedone commented Jan 19, 2024

Since we already introduce breaking changes in minor versions, which is a no-no with SemVer, why not just do it now. Any objections @Skrypt (since you added the Needs Triage label)?

@Skrypt
Copy link
Contributor

Skrypt commented Jan 20, 2024

I have no objection as I approved this PR a while ago.

@Piedone
Copy link
Member

Piedone commented Jan 22, 2024

I'd merge this @TFleury but there are merge conflicts. I'd also fix those but for that, you need to tick this checkbox: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests Could you please do that?

I can also pull the changes and merge them from a PR of my own, but that would remove you as the author.

@TFleury
Copy link
Contributor Author

TFleury commented Jan 22, 2024

@Piedone I don't have the checkbox but I solved the conflicts.

@Piedone Piedone merged commit da845ea into OrchardCMS:main Jan 22, 2024
3 checks passed
@Piedone
Copy link
Member

Piedone commented Jan 22, 2024

Great, thank you!

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.

6 participants