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

Issue #175 - implement array with indicator indentation flag #227

Merged
merged 1 commit into from
Oct 27, 2020
Merged

Issue #175 - implement array with indicator indentation flag #227

merged 1 commit into from
Oct 27, 2020

Conversation

dswiecki
Copy link
Contributor

Solves #175

@cowtowncoder
Copy link
Member

Sounds like a useful addition (haven't reviewed change in detail but makes sense).
Just one quick note: since it is an API addition, it must go in a new minor version; in this case 2.12.0.
Timing works well as I am about to release first release candidate.

So, branch to merge would need to be 2.12.

@cowtowncoder cowtowncoder added the hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards label Oct 2, 2020
*<p>
* Default value is `false` for backwards compatibility
*
* @since 2.11.3
Copy link
Member

Choose a reason for hiding this comment

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

As per not on PR itself, new features need to be added in a minor version, not patch (as per SemVer guidance)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to 2.12.0

@cowtowncoder
Copy link
Member

Ok; ready to merge once I get CLA (see notes on issue), and PR gets rebased against 2.12 (to add a new feature).

Thank you once again for contributing this!

@dswiecki dswiecki changed the base branch from 2.11 to 2.12 October 26, 2020 19:23
@dswiecki
Copy link
Contributor Author

Updated PR, unfortuanetely can't rename branch - it has to be named 2.11 but it's based on 2.12. I've also sent CLA.

@cowtowncoder
Copy link
Member

@dswiecki np wrt name, only matters what it merges against. Will have a quick re-read; received CLA.

@cowtowncoder cowtowncoder merged commit 1331fc2 into FasterXML:2.12 Oct 27, 2020
@cowtowncoder
Copy link
Member

@dswiecki Works great for 2.12, although there is a problem with 3.0 (master) -- looks like the new snakeyaml-engine does not have method setIndentWithIndicator in DumpSettingsBuilder, and I am not sure what the equivalent might be.
I was wondering if you know what might be the replacement there? If not, we can ask @asomov for help too.

@asomov
Copy link
Contributor

asomov commented Oct 27, 2020

@dswiecki Indeed, this is not yet ported to snakeyaml-engine. I will do as soon as I can. Feel free to create an issue for snakeyaml-engine: https://bitbucket.org/asomov/snakeyaml-engine/issues?status=new&status=open

@cowtowncoder
Copy link
Member

@asomov
Copy link
Contributor

asomov commented Nov 12, 2020

@cowtowncoder delivered in version 2.2
https://search.maven.org/artifact/org.snakeyaml/snakeyaml-engine/2.2/bundle

@cowtowncoder
Copy link
Member

@asomov Excellent, update master to use 2.2, option added!

@asomov
Copy link
Contributor

asomov commented Nov 25, 2020

@cowtowncoder I am terribly sorry. Even though we have high test coverage a regression happen in SnakeYAML Engine version 2.2
New lines in the Windows format (\r\n) are not always respected properly.
There is already a fix in version 2.2.1
Can you please use the fixed version of SnakeYAML Engine ? Should I create a PR for the version change ?

@cowtowncoder
Copy link
Member

@asomov Sure PR would be good if you have time.

@asomov
Copy link
Contributor

asomov commented Nov 26, 2020

@cowtowncoder done: #234

@dswiecki dswiecki deleted the 2.11 branch December 1, 2020 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Issue related to Hactoberfest2020 activities, eligible for additional rewards yaml Issue related to YAML format backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants