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

Feature/exportToXML #62

Merged
merged 8 commits into from
Sep 6, 2017
Merged

Feature/exportToXML #62

merged 8 commits into from
Sep 6, 2017

Conversation

rodionovsasha
Copy link
Owner

No description provided.


@NoArgsConstructor
@XmlRootElement(name = "instructions")
abstract class JFixturesResultImpl implements JFixturesResult {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's rename it to JFixturesResultBase since it is not an implementation but an abstract class?

abstract class JFixturesResultImpl implements JFixturesResult {
private String fixturesFolder;

@Setter
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need @Setter? Can't we put XML related annotation on getter(getInstructions() method) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If getter is an allowed mechanism, then, perhaos, we can remove this field at all or leave only getInstructions()?

@@ -24,6 +26,11 @@ public FixtureValue(Object value) {
}
}

@XmlValue
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason not to serialize the type field?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do you mean
@XmlValue private final Object value;

@@ -24,6 +26,11 @@ public FixtureValue(Object value) {
}
}

@XmlValue
public String getStringValue() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it serialize if the method is defined like Object getValue()?

Copy link
Owner Author

Choose a reason for hiding this comment

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

`fixturesResult.asString() == EXPECTED_XML
| |
| java.lang.NullPointerException
com.github.vkorobkov.jfixtures.fluent.impl.XmlJFixturesResultImpl@548a24a

at com.github.vkorobkov.jfixtures.fluent.impl.XmlJFixturesResultImplTest.processes fixtures to string(XmlJFixturesResultImplTest.groovy:57)`


@NoArgsConstructor
@AllArgsConstructor
@XmlRootElement(name = "instructions")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can @XmlRootElement be moved into XmlJFixturesResultImpl?


@SneakyThrows
private Marshaller getMarshaller() {
val jaxbContext = JAXBContext.newInstance(JFixturesResultBase.class);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be instantiated like JAXBContext.newInstance(getClass())?

@@ -29,6 +31,11 @@ public String toString() {
return String.valueOf(value);
}

@XmlValue
String getValueAsString() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename to getXmlRepresentation?

@@ -4,6 +4,8 @@
import lombok.EqualsAndHashCode;
import lombok.Getter;

import javax.xml.bind.annotation.XmlValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we don't add type field into result XML?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(as a attribute/tag)

Copy link
Collaborator

Choose a reason for hiding this comment

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

So it could be <value type="SQL">DEFAULT</value>

Copy link
Collaborator

@vkorobkov vkorobkov left a comment

Choose a reason for hiding this comment

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

Minor remarks and questions left

@@ -4,6 +4,8 @@
import lombok.EqualsAndHashCode;
import lombok.Getter;

import javax.xml.bind.annotation.XmlValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(as a attribute/tag)

@@ -4,6 +4,8 @@
import lombok.EqualsAndHashCode;
import lombok.Getter;

import javax.xml.bind.annotation.XmlValue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

So it could be <value type="SQL">DEFAULT</value>

abstract class JFixturesResultBase implements JFixturesResult {
private String fixturesFolder;

@XmlElements({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this XML specific annotation to XmlJFixturesResultImpl since the base class should not know about the implementation and technical details of its children?

@Override
@XmlElements( .... )
List<Instruction> getInstructions() {
  return super.getInstructions();
}

Copy link
Owner Author

Choose a reason for hiding this comment

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

💯

Copy link
Collaborator

@vkorobkov vkorobkov left a comment

Choose a reason for hiding this comment

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

Looks amazing! 🍰

@rodionovsasha rodionovsasha merged commit e0905a2 into master Sep 6, 2017
@rodionovsasha rodionovsasha deleted the feature/export branch September 6, 2017 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants