-
-
Notifications
You must be signed in to change notification settings - Fork 412
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
[YamlSourceManipulator] Tweak regex pattern for regex key #910
[YamlSourceManipulator] Tweak regex pattern for regex key #910
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! This is awesome! Could you also add a test case? These are fairly simply - there is a directory of test cases - https://github.com/symfony/maker-bundle/tree/main/tests/Util/yaml_fixtures - if you created a new file here, it would run :)
Thanks!
autowire: true | ||
autoconfigure: true | ||
bind: | ||
$httpClient: '@http.client' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation is slightly off here, which is making the test fail (it's making the expected not match)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I miss this thank you
arguments: | ||
- argument_1: 'something' | ||
argument_2: 'something' | ||
argument_3: 'something' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing. The bug you're fixing is related to the $httpClient
above. Unfortunately, this array syntax down here (while totally legal) is, I think, not yet supported by YamlSourceManipulator
. And so, the test is STILL failing. I'd recommend updating this test to not use this syntax - instead use:
app.service:
class: App\Services\Service
arguments:
argument_1: 'something'
argument_2: 'something'
argument_3: 'something'
If you're interested, you could open a separate PR with a failing test case for this array syntax :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see Thank you, I just tried somethings and changed it how you propose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@weaverryan Can you take a look again, please
Thank you
f6e8a43
to
da120f1
Compare
@lubo13 Could you rebase this PR against the |
@jrushlow can you check now, please. 10x in advance |
e854ce9
to
055abf9
Compare
055abf9
to
4f54c56
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @lubo13 for the contribution!
6d16253
to
85ce7c5
Compare
We need to tweak a regex pattern, because at the moment if you try to update yaml with following config:
You will receive: