Skip to content

Add FlowStyleResolver to enable custom YAML node style resolution #242

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

Closed
wants to merge 2 commits into from

Conversation

axpendix
Copy link

@axpendix axpendix commented Feb 3, 2021

This adds a desired mechanism to specify YAML node styles (flow styles in snakeyaml) for individual object and array nodes, via implementing a FlowStyleResolver and checking the field names of given object or array nodes.

References: #185 #133 #43

This adds a desired mechanism to specify YAML node styles (flow styles in snakeyaml) for individual object and array nodes, via implementing a FlowStyleResolver and checking the field names of given object or array nodes.
@cowtowncoder
Copy link
Member

@axpendix Apologies for slow follow up here: will try to get this done soon now (hopefully today). Quick question: did I mention CLA earlier? If not, I'd need this:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and after I get it, can merge. CLA only needs to be sent once so if this was already taken care of no need to re-send (I just need to locate it).
If you do need to send it, the common way is to print, fill & sign, scan/photo, email to info at fasterxml dot com.

Looking forward to merging this!

* typically root object)
* @return the desired {@link NodeStyle} or null to use default value (currently 'BLOCK')
*/
NodeStyle resolveStyle(String fieldName);
Copy link
Member

Choose a reason for hiding this comment

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

I realized that instead of passing just fieldName, it'd be more flexible to pass write context; and perhaps also have 2 methods to call: one for Objects, another for Arrays.
I guess it would be possible to pass both name (current name in context) and context, to simplify some cases, while giving full power for others.

At very least I think array and object cases should go to different methods since I think that's relatively common use case.

Copy link
Member

Choose a reason for hiding this comment

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

... just realized I added this note already. Wrt comment I just added above.

And looking at this method now, maybe passing the default style is not useful after all (because it would need to be translated etc). Returning null makes sense.

String yamlTag = _typeId;
boolean implicit = (yamlTag == null);
String anchor = _objectId;
if (anchor != null) {
_objectId = null;
}
NodeStyle jacksonStyle = _nodeStyleResolver.resolveStyle(
Copy link
Member

Choose a reason for hiding this comment

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

Would make sense to use getCurrentName() bit earlier here, before createChildArrayContext().

Would this work with nested arrays? (might be that it gets null).

Also: would make sense to get and pass default style to custom resolver -- but resolver could be passed the context as well (pre-addition of array), to let it access all information, including ancestors. I'll add a note on interface later on.

@cowtowncoder
Copy link
Member

@pjfanning What do you think about this earlier PR? I don't think I have gotten a CLA to merge this, but I suppose that exposing more of SnakeYAML's functionality might make sense. Not sure how well it'd merge with snakeyaml-engine in 3.0 but otherwise.

@pjfanning
Copy link
Member

@cowtowncoder I haven't really messed about with these features of snakeyaml. The only reason I wanted to expose the LoaderOptions was because the default 3Mb limit for parsing is quite small.

@cowtowncoder
Copy link
Member

@pjfanning sure, understood. And changes here are bit more involved as the handler can change settings on per-property name basis, which should be tested etc.

@pjfanning
Copy link
Member

pjfanning commented Sep 25, 2022

@cowtowncoder one thing we could do is to allow users to set their own DumperOptions on YAMLFactory (like they can now with LoaderOptions). If users provide their own non-null DumperOptions, then https://github.com/FasterXML/jackson-dataformats-text/blob/2.14/yaml/src/main/java/com/fasterxml/jackson/dataformat/yaml/YAMLGenerator.java#L294 will be skipped. Would this make sense as v2.14 change?

I've added #345 to show how this might work. This new PR does not let you have different yaml flow styles depending on the element name though (like this PR 242 is about).

@cowtowncoder
Copy link
Member

@pjfanning I think that is a good starting point and probably covers many use cases even if not that particular one.

@asomov
Copy link
Contributor

asomov commented Jan 10, 2023

@axpendix do you think that something can be improved in SnakeYAML to support it ?
Is this similar ?

@cowtowncoder cowtowncoder added the yaml Issue related to YAML format backend label Mar 9, 2023
@cowtowncoder cowtowncoder added the cla-needed PR looks good (although may also require code review), but CLA needed from submitter label Jul 28, 2023
@cowtowncoder
Copy link
Member

No progress in years, unfortunately, against obsolete branch. Closing; may be re-filed against newer branch etc.

@asomov
Copy link
Contributor

asomov commented Aug 22, 2024

no progress also in SnakeYAML:
https://bitbucket.org/snakeyaml/snakeyaml-engine/issues/24/allow-flowstyle-property-to-be-configured

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-needed PR looks good (although may also require code review), but CLA needed from submitter yaml Issue related to YAML format backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants