Skip to content

[Yaml] Implement $blockChompingIndicator for TaggedValue multi-line literal blocks #40431

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: 7.4
Choose a base branch
from
Open

[Yaml] Implement $blockChompingIndicator for TaggedValue multi-line literal blocks #40431

wants to merge 5 commits into from
+60 −19

Conversation

natepage
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT

4c513c2 introduced the notion of $blockChompingIndicator for multi-line literal blocks.

This PR implements the same logic for TaggedValue multi-line literal blocks.

@natepage natepage requested a review from xabbuh as a code owner March 10, 2021 07:01
@carsonbot carsonbot added this to the 4.4 milestone Mar 10, 2021
@carsonbot carsonbot changed the title [YAML] Implement $blockChompingIndicator for TaggedValue multi-line literal blocks Implement $blockChompingIndicator for TaggedValue multi-line literal blocks Mar 10, 2021
@carsonbot carsonbot changed the title Implement $blockChompingIndicator for TaggedValue multi-line literal blocks [Yaml] Implement $blockChompingIndicator for TaggedValue multi-line literal blocks Mar 10, 2021
@natepage
Copy link
Contributor Author

@xabbuh Hi, not sure why Tests/Integration (7.4) is failing, any idea what I could have done wrong?

@nicolas-grekas
Copy link
Member

Can you please add a few more test cases to cover all the branches?

@natepage
Copy link
Contributor Author

natepage commented Mar 17, 2021 •
edited
Loading

@nicolas-grekas Thank you for your reply, sorry I just want to clarify.. What do you mean by "cover all the branches"?

  • Are you talking about repo branches for different versions of symfony? (e.g. 4.4, 5.0, 5.1, 5.2)
  • Or, are you talking about "code branches/paths" and improve the coverage?

Thank you

@nicolas-grekas
Copy link
Member

Or, are you talking about "code branches/paths" and improve the coverage?

that yes :)

@xabbuh
Copy link
Member

xabbuh commented Mar 18, 2021

@natepage You can ignore that build failure. It's not related to your changes.

@xabbuh
Copy link
Member

xabbuh commented Mar 26, 2021

@natepage Do you need help with writing a test? :)

@natepage
Copy link
Contributor Author

@xabbuh I should be ok for the test, any tips on raising a kid? 😄

Sorry I had a few concurrent priorities just after creating the PR.

Having a look at the code, I've realised there was a difference between scalar and tagged values regarding trailing newlines so I've updated the code to match, and reused your test testDumpTrailingNewlineInMultiLineLiteralBlocks() but with TaggedValues and make sure the result is consistent.

@nicolas-grekas The Dumper is now 100% covered.

xabbuh
xabbuh reviewed Mar 29, 2021
View reviewed changes
stof
stof reviewed Apr 1, 2021
View reviewed changes
@natepage natepage requested a review from xabbuh April 22, 2021 12:45
@natepage natepage requested a review from stof May 3, 2021 14:02
@OskarStark
Copy link
Contributor

@xabbuh @stof open for a final review? CI is green

@fabpot
Copy link
Member

fabpot commented Aug 26, 2021

As this is a new feature, this should target the main branch (5.4 as of today).

@fabpot fabpot modified the milestones: 4.4, 5.4 Aug 26, 2021
@natepage natepage changed the base branch from 4.4 to 5.4 August 26, 2021 21:56
@natepage
Copy link
Contributor Author

@fabpot Thank you for your feedback, I've changed the base branch to 5.4

@fabpot fabpot removed this from the 5.4 milestone Nov 16, 2021
@fabpot fabpot added this to the 6.1 milestone Nov 16, 2021
@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@fabpot
Copy link
Member

fabpot commented Apr 8, 2023

@xabbuh Can you have a look at this one please?

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxhelias maxhelias maxhelias approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh xabbuh is a code owner

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Bug Status: Reviewed Yaml
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
8 participants
@natepage @nicolas-grekas @xabbuh @OskarStark @fabpot @stof @maxhelias @carsonbot