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

add merge-replace config #233

Merged
merged 3 commits into from
Sep 1, 2022
Merged

add merge-replace config #233

merged 3 commits into from
Sep 1, 2022

Conversation

mjauvin
Copy link
Contributor

@mjauvin mjauvin commented Jun 26, 2022

Fix #221

This allows to control merging of composer replace section.

Copy link

@LukeTowers LukeTowers left a comment

Choose a reason for hiding this comment

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

Not that my approval is worth anything here, but this is a simple change with limited to no side effects.

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 8, 2022

Is this project still maintained? There has been next to no activity for the past year.

@LukeTowers
Copy link

I've just gone with the approach of not including the Winter plugins in my projects' merge config.

Copy link
Member

@reedy reedy left a comment

Choose a reason for hiding this comment

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

Thanks for the patch. merge-replace needs documenting in README.md like other settings.

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 23, 2022

@reedy I documented merge-replace in README.md

Thanks.

Copy link
Member

@reedy reedy left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry to be a pain. Can we also mark it as optional in README.md (line 111) like we do for other similar config for consistency?

Like:

* [require-dev](https://getcomposer.org/doc/04-schema.md#require-dev)
(optional, see [merge-dev](#merge-dev) below)

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 23, 2022

@reedy sorry, I checked the README.md file at line 111 and I don't understand what you mean by "make it optional"

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 23, 2022

@reedy If you mean the "*" in front of replace on line 111, this is for a bulletted list... not the usual "this is optional".

@reedy
Copy link
Member

reedy commented Aug 23, 2022

Screenshot 2022-08-23 at 16 39 14

For the other options where they have a config switch, like you're doing for replace, we've got documented that you can turn it on an off, including a link to the documentation section you added in the previous revision

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 23, 2022

@reedy sorry, brain cramp this morning... commit has been added for this.

Thanks.

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 29, 2022

@reedy anything else required to be modified for this?

@mjauvin mjauvin requested a review from reedy August 29, 2022 12:35
@reedy
Copy link
Member

reedy commented Aug 29, 2022

Not unless you want to write some tests to test the merge-replace: false state ;)

I'd like to try and get #240 fixed so CI is actually useful before merging more new features (even though this one is trivial)...

@mjauvin
Copy link
Contributor Author

mjauvin commented Aug 29, 2022

@reedy Sure, fix your CI and I'll write a testcase.

@LukeTowers
Copy link

@reedy unless you're planning on taking care of #240 in the next week or so can we just get this PR merged for now? 😄 I know how easy it is for things to slip by the wayside for the sake of wishful thinking, so if you're willing to I'd really rather just get this simple change merged and then look at fixing CI later when someone actually has time to do it.

@reedy
Copy link
Member

reedy commented Aug 30, 2022

Haha. Fair point. I'll do it later today.

The tests confuse me tbh (I didn't write them!), and unless someone else looks at them and has any idea any time soon...

@reedy reedy merged commit 7692a62 into wikimedia:master Sep 1, 2022
@eileenmcnaughton
Copy link

yay

@wgevaert
Copy link

great patch!
When will this be released into a new version?

@mjauvin
Copy link
Contributor Author

mjauvin commented Mar 2, 2023

@reedy any way to tag a release so this can be used in stable composer installation?

Thanks.

@reedy
Copy link
Member

reedy commented Apr 15, 2023

@reedy any way to tag a release so this can be used in stable composer installation?

Thanks.

Sorry for the slow reply... I've created https://github.com/wikimedia/composer-merge-plugin/tree/v2 to revert out the dropping of old PHP support (but no CI due to supported images by GH Actions) and testing of 8.2 (because of different library requirements) to remove the more breaking changes in master... Ideally the next major release wants even more breaking changes (ie dropping composer 1.x support (ala #243).. But those code updates haven't been done yet.

And then turned that into https://github.com/wikimedia/composer-merge-plugin/releases/tag/v2.1.0

@mjauvin
Copy link
Contributor Author

mjauvin commented Apr 15, 2023 via email

@reedy
Copy link
Member

reedy commented Apr 15, 2023

We don't need a release for it to work... They're primarily for binary releases etc - https://docs.github.com/en/repositories/releasing-projects-on-github/about-releases

https://packagist.org/packages/wikimedia/composer-merge-plugin#v2.1.0 shows it's there, so it can be installed just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add support for merge-replace option
5 participants