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

fixes #951: Updated grasmash/yaml-expander dependency #952

Closed
wants to merge 2 commits into from

Conversation

romanmartushev
Copy link

@romanmartushev romanmartushev commented Jun 3, 2020

Overview

This pull request:

  • Fixes a bug

Summary

Updated grasmash/yaml-expander dependency to version that supports symfony/yaml v5

@romanmartushev
Copy link
Author

closes #951

@alsar
Copy link

alsar commented Aug 24, 2020

@romanmartushev why was this closed?

@romanmartushev
Copy link
Author

The pipeline did not pass and I was not going to up the coverage just for a dependency change.

@greg-1-anderson
Copy link
Member

I haven't looked at this. In general, I am in favor of major version updates of dependencies if it can be done in a backwards-compatible way. If anyone wants to continue with this investigation, please also use the | operator so that existing supported versions continue to work. It is not necessary to support every intermediate version.

@alsar
Copy link

alsar commented Aug 24, 2020

@greg-1-anderson grasmash/yaml-expander v3.0 has a constraint "symfony/yaml": "^4 | ^5". Robo requires v4.4.8 as a minumim for Symfony components. So there should not be any backwards incompatibility with bumping the version for grasmash/yaml-expander.

@greg-1-anderson
Copy link
Member

gasmash/yaml-expander itself may have introduced bc-breaking changes in 2.x or 3.x. I have not investigated this.

@alsar
Copy link

alsar commented Aug 25, 2020

@greg-1-anderson according to changelogs there are none. At least they shouldn't affect Robo.
v2 had internal refactoring and v3 dropped support for php 5.4 and Symfony 2.

BTW, I don't see that this package is used anywhere in the codebase.

@greg-1-anderson
Copy link
Member

As I said above, I am in favor of major version upgrades of dependencies if there are no bc breaks because of it. This PR would be acceptable with passing tests. If grasmash/yaml-expander really is not used in this repository, then a PR that removed it as a dependency (and has passing tests) would also be acceptable.

It is plausible that grasmash/yaml-expander is no longer used here. Robo primarily uses consolidation/config for configuration access, which requires grasmash/expander instead.

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.

3 participants