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

Create a method like indented to XML serialization #612

Closed
renanigt opened this issue Jun 18, 2014 · 37 comments · Fixed by #613
Closed

Create a method like indented to XML serialization #612

renanigt opened this issue Jun 18, 2014 · 37 comments · Fixed by #613

Comments

@renanigt
Copy link
Contributor

Allways the result.use(xml()).from(object).serialize() brings in a pretty format what may difficult the tests and we may don't want for any reason.

What about create some method like indented of JSONSerialization ?

@renanigt renanigt changed the title Method like indent to XML serialization Create a method like indent to XML serialization Jun 18, 2014
@renanigt renanigt changed the title Create a method like indent to XML serialization Create a method like indented to XML serialization Jun 18, 2014
@Turini
Copy link
Member

Turini commented Jun 18, 2014

did you mean remove Gson prettyPrint() option by default?
And just use prettyPrint() if we do something like this? :

result.use(xml()).from(object).indent().serialize()

If so, I don't like this... I'd prefer +human readable output by default.

@Turini
Copy link
Member

Turini commented Jun 18, 2014

and it will break compability on it's released version.

@Turini
Copy link
Member

Turini commented Jun 18, 2014

ops.. forgot about Gson (you are talking about XStream).
but my point is the same... what do you think?

@renanigt
Copy link
Contributor Author

I think the just impact is on tests and don't is big, but is really too specific.

So may close this issue. 😃

@Turini
Copy link
Member

Turini commented Jun 18, 2014

what do you think @garcia-jj and @lucascs?

@garcia-jj
Copy link
Member

@Turini, we already have this discussion 2 or 3 years ago when develop
vraptor 3.

Json and XML are made to machine exchange, not to humans read. With no
spaces and break lines performance are increased a lot. And our decision
was made based on this.

If some dev needs to debug, there are many tools that already do this job
like firebug. And all browsers have tools that do this job too.

@Turini
Copy link
Member

Turini commented Jun 18, 2014

With no spaces and break lines performance are increased a lot.
And our decision was made based on this.

so why it appears formatted until now? Or it does not? (I'm really not sure)

@garcia-jj
Copy link
Member

Good question. I'm not in my computer right now. But later I can see this
case.

@renanigt
Copy link
Contributor Author

It's appear just formatted.

@Turini
Copy link
Member

Turini commented Jun 18, 2014

yeap... this is my point. Until now XML output is formatted.

@garcia-jj
Copy link
Member

May a bug. As you see we don't have "without indentation" method, only
"indented". So all serialization result should not indented.

Can you do a test to us? Create two methods that serializes to XML, one
that calls indented and another that don't call. How result XML appears?

And the same with json.

@renanigt
Copy link
Contributor Author

I'm not at home now, but after I'll do this.

@garcia-jj
Copy link
Member

Hmm, now I'm on a computer :happy:.

I remember that I ask this behavior on dev mailing list. And after a long discussion we decide that all serializations should be without indentation. Why? Because JSON and XML is just to machine exchange, not to be user readable. Without indentation we have more performance when download the JSON and XML. For a XML or Json, spaces/tabs/linebreaks is usefulness.

And now all browsers have native tools to pretty debug JSON or XML.

As you see there is no method to remove indentation, only methods to add indentation. Because by default serialization isn't indented. I did this code in vraptor 3, so I think that this code was missed. See this: caelum/vraptor@9a11124

Take a look in GsonJSONSerializationTest class. The method shouldSerializeAllBasicFields is not indented. But shouldSerializeAllBasicFieldsIdented is indented because indented method was called. By default our JSON serialization isn't indented.

But XStreamXMLSerializationTest, method shouldSerializeAllBasicFields don't calls indented, and the result is indented. Analyzing the code I see that indented has no effect. So it's a bug (or undesired behavior).

I think that we can change this behavior now, because this change wont break applications because spaces/tabs/linebreaks are not relevant to JSON/XML processors (it's just ignored). But can impact is users have tests comparing strings, so we can add a text explain this change in release notes.

@garcia-jj
Copy link
Member

I found the thread :) https://groups.google.com/forum/#!topic/caelum-vraptor-dev/FpVYFlzMNMI

But I think that we only change for JSON, not to XML :(

@mariofts
Copy link
Member

Can we have both worlds? In production the code is not identeted, and in
any other environment is, what do you think?

My other sugestion is not ident by default at all, like V3. Change the
default behaviour now and put a release note. This make a lot of difference
to apps with tons of requests.

Mário do Amaral Gonçalves

2014-06-18 11:35 GMT-03:00 Otávio Garcia notifications@github.com:

I found the thread :)
https://groups.google.com/forum/#!topic/caelum-vraptor-dev/FpVYFlzMNMI

But I think that we only change for JSON, not to XML :(


Reply to this email directly or view it on GitHub
#612 (comment).

@garcia-jj
Copy link
Member

Yes Mário, this is my point. Today we have not indented by default, and dev can call method indented if he wants indentation. It works fine with JSON, but in XML this option is not implemented (I think that we miss to do this).

@garcia-jj
Copy link
Member

Mário, what you think about a option like indentedIf(AN_ENVIRONMENT)?

@renanigt
Copy link
Contributor Author

It's a good idea @garcia-jj ! 👍

@felipeweb
Copy link
Contributor

i think we can create a environment option.
if in the file. properties are not explicitly which the developer's choice, we set not indened by default

what do you think?

@renanigt
Copy link
Contributor Author

I prefer something like indentedIf(AN_ENVIRONMENT) as was cited by @garcia-jj !
It's cleaner and simple to use it.

@mariofts
Copy link
Member

I prefer the Felipe approach. Something like identXml=false/identJson=false
in the default properties file. I can think in a situation where someone
need the output idented based on environment other than debug in
development, which is the environment responsability.

What I mean is this options shouldn't be in the code, if the purpose is
explicity debug.

Mário do Amaral Gonçalves

2014-06-18 12:25 GMT-03:00 Renan Montenegro notifications@github.com:

I prefer something like indentedIf(AN_ENVIRONMENT) as was cited by
@garcia-jj https://github.com/garcia-jj !
It's cleaner and simple to use it.


Reply to this email directly or view it on GitHub
#612 (comment).

@garcia-jj
Copy link
Member

All vraptor configuration are located at Java code. Can be easily
refactored, more readable, and more.

Just my opinion: properties can forgettable, and dev must see the key in
the docs. Placed in the class is more fluent and easily to see.

And I can add only in places I want. In the properties all places will
inherit the behavior.

I prefer method, but I don't see a big problem to use property. But I
dislike :)

May we can invite more people to discuss about. @Turini @lucascs @rponte

@rponte
Copy link
Contributor

rponte commented Jun 18, 2014

Why not both?

Having these properties in configuration file is good if you have to deal with multiple environments, so you just need to change the file in build or deploy-time, for example. But I agree with @garcia-jj that having an API is also a good idea and is the VRaptor way of developing.

@garcia-jj
Copy link
Member

If we use both, indentIf(Environment) needs to have higher priority than configuration. Makes sense?

@renanigt
Copy link
Contributor Author

Good idea @rponte. 👍
In this case, we just need decide the order of priority.

@renanigt
Copy link
Contributor Author

For me makes sense @garcia-jj !

@lucascs
Copy link
Member

lucascs commented Jun 19, 2014

text configuration usually takes precedence over programatic/annotation configuration... so you can create a properties/xml for each environment and don't have to recompile a jar.

I wouldn't put the indentedIf method, though...

@garcia-jj
Copy link
Member

@lucascs when I wrote the 1st piece of this code I think that same you.

@garcia-jj
Copy link
Member

Reopen to add support for environment.

@garcia-jj garcia-jj reopened this Jun 19, 2014
@renanigt
Copy link
Contributor Author

MockSerializationResult is affected with this improvement ?

@garcia-jj
Copy link
Member

Nope.

@renanigt
Copy link
Contributor Author

I think that a good idea would be MockSerializationResult doesn't indent the result either.
So, make tests with no indent is easier.

What do you think ?

@garcia-jj
Copy link
Member

Redundant, because user can add this config via environment. (test.properties when use tests).

@renanigt
Copy link
Contributor Author

Oh, really, will be via environment.

Thanks

@renanigt
Copy link
Contributor Author

I thought about this because MockSerializationResult use XStreamBuilderImpl and this class doesn't indent the result either.

Sorry for the doubts.

@renanigt
Copy link
Contributor Author

Sorry, I was wrong.

@garcia-jj
Copy link
Member

Implemented, and will be available in the next release. Thank you, people, to your suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants