Closed GoogleCodeExporter closed 9 years ago
loadAs() methods can be implemented with a minor backwards incompatible change.
See http://code.google.com/r/py4fun-loadasbeans/source/list
Original comment by py4fun@gmail.com
on 4 Jul 2011 at 6:31
The change looks good so far: good work. I only have one concern with the
change so far, which is that JavaBeanLoader lets you use TypeDescription for
the root type, but there are no loadAs() methods to replace that function. If
that change is intentional I don't see a problem with it.
Original comment by JordanAn...@gmail.com
on 4 Jul 2011 at 8:11
Thank you Jordan for the idea!
I did not quite catch your concern. In fact JavaBeanLoader did not use the
whole TypeDescription for the root type. It was just setting the tag for the
root Node.
(I am afraid I might misunderstand your question.)
Original comment by aso...@gmail.com
on 4 Jul 2011 at 9:20
You understood me perfectly, I just hadn't looked closely enough at what
JavaBeanLoader did. It looks like we don't need methods to deal with
TypeDescription, which is good (keeps the API complexity down).
While we're removing deprecated classes, we may as well remove JavaBeanParser,
as it has been deprecated for almost two years now.
Original comment by JordanAn...@gmail.com
on 4 Jul 2011 at 9:31
I agree: JavaBeanParser has been removed.
Original comment by py4fun@gmail.com
on 5 Jul 2011 at 7:11
It is unclear what to do with JavaBeanDumper. I would like to deprecate it. All
its business is just to set two settings in DumperOptions:
setExplicitRoot(Tag.MAP);
setDefaultFlowStyle(flowStyle);
Original comment by py4fun@gmail.com
on 5 Jul 2011 at 9:17
I would suggest deprecated in 1.9, removed in 2.0.
Original comment by JordanAn...@gmail.com
on 5 Jul 2011 at 8:07
What I mean is: I do not know how to apply settings used in JavaBeanDumper. In
order to deprecate it we need to provide a better way to serialize a JavaBean.
What it should be ? How to dump a JavaBean ? I would like to have a simple way
and avoid the root tag with the class name because it is redundant.
Original comment by py4fun@gmail.com
on 6 Jul 2011 at 6:56
what about something similar to loadAs(...) ?
Original comment by alexande...@gmail.com
on 6 Jul 2011 at 9:39
About dumpAs()
1) Yaml instance has its own DumperOptions. If in this DumperOptions there is a
custom setting for the root tag or for the flow style which setting should be
more important ?
2) dumpAs is not a good name because there will be no additional argument with
a class.
So it can be named 'dumpClass'. Since it will have the same argument(s) as
existing 'dump' methods, it may be confusing for users which one to pick up.
I think there shall be a very easy way to provide a proper DumperOptions
instance. But how ?
Original comment by py4fun@gmail.com
on 6 Jul 2011 at 11:17
My opinion is that dumpAs() should take a Tag as the second argument. For
example, JavaBeanDumper's current behaviour would be similar to: yaml.dumpAs (
object, Tag.MAP );
I don't think the configuration (DumperOptions settings) applied in
JavaBeanDumper is particularly special. A little bit of documentation showing
users how to change these settings for a Yaml object is better than bothering
to duplicate this behaviour directly. Users can provide a DumperOptions
instance to Yaml as a constructor argument.
The big thing that can only be easily done through JavaBeanDumper is to dump
something without a root level tag, or with a different root level tag. This is
what we must duplicate in dumpAs().
Original comment by JordanAn...@gmail.com
on 6 Jul 2011 at 12:39
yaml.dumpAs(object, Tag.MAP) is also confusing: it looks like users are free to
define the second argument. But in fact, we want to enforce 'Tag.MAP'. What
happens if they say yaml.dumpAs(object, Tag.SEQ) ? What do we do with the flow
setting?
We can stick to 'yaml.dumpClass(object)' but what happens if users try to dump
a list with such a method ?
Original comment by py4fun@gmail.com
on 6 Jul 2011 at 3:38
Do we necessarily want to enforce Tag.MAP? What if I want to dump some object
with a custom Represent instance as a Tag.SEQ? Or as Tag.PAIRS?
If you say yaml.dumpAs(object, Tag.SEQ), then object is dumped with the
sequence tag, like you asked it to do. It may not be possible for you to load
it back easily, but it did exactly what you told it to do.
I can imagine a circumstance where I have an object (MyObject), with a custom
Represent instance, that I would like to dump tagged as a Sequence:
yaml.dumpAs(myObject, Tag.SEQ) . The custom Represent instance returns a
SequenceNode. Then I can use yaml.loadAs(document, MyObject.class) to load the
object with the correct type, but the document can still be loaded as a
Sequence (list, array, whatever) if that type is not available.
Ultimately, more thought needs to go into how nodes are tagged in output, both
for cross-language communication and for customization. However, this is a
good, simple start.
Original comment by JordanAn...@gmail.com
on 6 Jul 2011 at 4:01
Should we introduce a factory which creates Yaml instances configured for
different needs ? Then it is easy to get an instance where DumperOptions object
is pre-configured to serialize JavaBeans ? (with no root tag and with better
flow)
Original comment by py4fun@gmail.com
on 7 Jul 2011 at 8:11
I don't think so. If anything, a DumperOptions factory would make more sense,
but this is such a small bit of configuration that there's no point creating
more code to maintain it. Writing a comment or some documentation on how to
return to the old behaviour (supply a DumperOptions with FlowStyle set...)
would be better.
Original comment by JordanAn...@gmail.com
on 7 Jul 2011 at 2:50
I think we may also make setDefaultFlowStyle(FlowStyle.BLOCK) the default
setting. Then only one setting is left - for the root tag.
Original comment by py4fun@gmail.com
on 11 Jul 2011 at 10:59
I support changing the default flow style to FlowStyle.BLOCK .
Original comment by JordanAn...@gmail.com
on 11 Jul 2011 at 8:42
I agree with FlowStyle.BLOCK by default.
Original comment by alexande...@gmail.com
on 13 Jul 2011 at 9:20
96 tests fail when I change the default to FlowStyle.BLOCK. It will take quite
some time to apply the change in all the tests...
Original comment by py4fun@gmail.com
on 13 Jul 2011 at 9:43
I have made FlowStyle.BLOCK the default in the clone:
http://code.google.com/r/py4fun-deprecate-javabeandumper/source/checkout
This is how JavaBeans should be dumped:
DumperOptions options = new DumperOptions();
options.setExplicitRoot(Tag.MAP);
Yaml yaml = new Yaml(options);
Unfortunately I see 2 inconveniences:
1) very verbose
2) Tag.MAP is exposed. I am not sure this is good.
Original comment by py4fun@gmail.com
on 13 Jul 2011 at 4:45
This is verbose only because DumperOptions is getting involved. If the explicit
root is handled as a parameter for a "dump" operation (that is, as a method
argument to dumpAs()), then it will not be verbose.
I don't see a problem with exposing Tag.MAP. Plenty of users already create
their own custom tags, and everyone who is using YAML should be aware of enough
of the standard to know that tags exist, that the default tag for a mapping is
!!map, etc.
Original comment by JordanAn...@gmail.com
on 15 Jul 2011 at 4:56
Do you mean we should introduce:
Yaml.dumpAs(data, tag)
Yaml.dumpAs(data) - which simply calls Yaml.dumpAs(data, Tag.MAP)
Original comment by py4fun@gmail.com
on 15 Jul 2011 at 5:37
No, I think Yaml.dumpAs(data, tag) is sufficient. Without the second argument,
"dumpAs" doesn't make sense as the method name.
Original comment by JordanAn...@gmail.com
on 15 Jul 2011 at 5:58
I think I would prefer now your proposal with 'Yaml.dumpAs(data, Tag.MAP)'
Let us wait what Alex says. If he also does not object then I will drop my
experiment and introduce this method in master.
Original comment by py4fun@gmail.com
on 15 Jul 2011 at 9:19
I have put the implementation for 'Yaml.dumpAs(data, Tag.MAP)' to
http://code.google.com/r/py4fun-make-dump-as-method/source/list
Please have a look.
Original comment by py4fun@gmail.com
on 16 Jul 2011 at 1:58
Looks fine.
The only think I am not so sure:
* @param rootTag
* the tag for the whole YAML document. The tag must be Tag.MAP
* for a JavaBean.
The tag must be Tag.MAP far a JavaBean. Is it really MUST ?
A thought Tag.MAP will just make root tag disappear for JavaBean.
I can easily put there any other custom tag and maybe intentionally, e.g. I
want different, specific class to be used when document loaded...
So I agree with a change. Would be nice if we can change javadoc. Unfortunately
I do not know how ;)
Original comment by alexande...@gmail.com
on 18 Jul 2011 at 10:03
What about this:
"the tag for the whole YAML document. The tag should be Tag.MAP for a JavaBean
to make the tag disappear (to use implicit tag !!map)"
Original comment by py4fun@gmail.com
on 18 Jul 2011 at 10:10
Feels a bit better, but I am not sure.
Jordan, what do you think?
I guess among us you have the best knowledge in English.
Original comment by alexande...@gmail.com
on 18 Jul 2011 at 10:27
Since the only question left is the JavaDoc contents I will promote the change
to the master. Then everyone can adapt the JavaDoc.
Original comment by py4fun@gmail.com
on 18 Jul 2011 at 11:34
I do see some problems in the existing code (sorry I didn't reply earlier, I
had a busy weekend):
If I create a DumperOptions and give it to two Yaml instances, then call
Yaml.dumpAs(Object,Tag) on one, then the root tag and flow style will change
for BOTH Yaml instances.
If I create a new Yaml object, then call Yaml.dumpAs(Object,Tag), then call
Yaml.dump(Object), the second dump call will have the same explicit root tag
and flow style as in the call to dumpAs.
These problems both arise from the same issue: we should not change
configurations in a dumpAs() call. In fact, we should probably never change
configurations that we get from the user. If we absolutely MUST change
configurations, then we should change them back as soon as we are done.
I think a more verbose JavaDoc is best:
/**
* <p>Serialize a Java object into a YAML string. Override the default root tag
* with <code>rootTag</code>.</p>
*
* <p>This method is similar to <code>Yaml.dump(data)</code> except that the
* root tag for the whole document is replaced with the given tag. This has two
* main uses.</p>
*
* <p>First, if the root tag is replaced with a standard YAML tag, such as
* <code>Tag.MAP</code>, then the object will be dumped as a map. The root tag
* will appear as <code>!!map</code>, or blank (implicit !!map).</p>
*
* <p>Second, if the root tag is replaced by a different custom tag, then the
* document appears to be a different type when loaded. For example, if an
* instance of MyClass is dumped with the tag !!YourClass, then it will be
* handled as an instance of YourClass when loaded.</p>
*
* @param data
* Java object to be serialized to YAML
* @param rootTag
* the tag for the whole YAML document. The tag must be Tag.MAP
* for a JavaBean.
*
* @see Yaml.loadAs(String, Tag)
* @see Tag
*
* @return YAML String
*/
Original comment by JordanAn...@gmail.com
on 18 Jul 2011 at 1:28
1) I will apply this JavaDoc. Thank you.
2) I totally agree that Yaml.dumpAs() should have no side effects. But how can
we implement it ?
Original comment by py4fun@gmail.com
on 18 Jul 2011 at 2:01
I can't say what the ideal implementation should be. However, I can say how I
did this in my experimental clone:
1. Remove explicit root from configuration objects (DumperOptions).
2. Wherever an explicit root tag is required or accessed, make it a method
argument or constructor argument. For example, Serializer looks at
DumperOptions for an explicit root; instead, it should accept an explicit root
as a constructor argument.
3. Rearrange code to pass explicit root tags as required. An example of how I
handled dumpAs() can be found here:
http://code.google.com/r/jordanangold-enhanced-usability/source/browse/src/main/
java/org/yaml/snakeyaml/YamlDumper.java . There is probably a simpler way to
implement that.
A cheap hack that would solve one of the problems:
public String dumpAs(Object data, Tag rootTag) {
Tag oldRoot = dumperOptions.getExplicitRoot();
FlowStyle oldFlow = representer.getDefaultFlowStyle();
dumperOptions.setExplicitRoot(rootTag);
representer.setDefaultFlowStyle(FlowStyle.BLOCK);
String result = dump(data);
dumperOptions.setExplicitRoot(oldRoot);
representer.setDefaultFlowStyle(oldFlow);
return result;
}
However, this does not solve the problem when a single DumperOptions instance
is shared between Yaml instances.
Original comment by JordanAn...@gmail.com
on 18 Jul 2011 at 2:34
I could get rid of the side effects for Yaml.dumpAs(). I have added yet another
method. Can you please have a look?
(explicit root from DumperOptions has become deprecated)
http://code.google.com/r/py4fun-no-side-effects/source/list
Original comment by py4fun@gmail.com
on 22 Jul 2011 at 2:17
I suppose that's a working solution. Maybe not the best solution, but I'm sure
we can incrementally improve upon it.
I would suggest that we define more accurately what happens when either the
root tag or the flow style is null. For flow style, it should just use whatever
is already in the Representer; for tag, it should just use the default tag
(that is, no explicit root).
Also, "dumpAs(Object data)" would probably be better if named
"dumpAsBean(Object data)".
Original comment by JordanAn...@gmail.com
on 22 Jul 2011 at 2:32
Looks OK for me.
dumpAs(Object) a bit confusing name, but dumpAsBean(Object) isn't better,
dumpAsMap(Object) might be at least more close to the real thing.
Original comment by alexande...@gmail.com
on 22 Jul 2011 at 2:52
I was too quick to apply dumpAsBean(Object) :) It was already pushed to the
clone
I agree that dumpAsMap(Object) is better.
If we share the same view I will apply dumpAsMap(Object) in the master
repository (thanks to Mercurial)
Original comment by py4fun@gmail.com
on 22 Jul 2011 at 3:23
I agree: dumpAsMap(Object) is a better choice.
Original comment by JordanAn...@gmail.com
on 22 Jul 2011 at 3:52
dumpAsMap(Object) applied.
The change will be delivered in version 1.9
Original comment by py4fun@gmail.com
on 22 Jul 2011 at 4:08
Original issue reported on code.google.com by
py4fun@gmail.com
on 4 Jul 2011 at 11:33