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

Improve handling of XML attributes and element values. #394

Merged
merged 7 commits into from
Sep 28, 2021

Conversation

blackwinter
Copy link
Member

@blackwinter blackwinter commented Sep 24, 2021

Make attribute markers configurable:

Make value tag names configurable:

Fixes #379.

Related to #336.

@blackwinter blackwinter requested a review from dr0i September 24, 2021 18:14
@blackwinter blackwinter assigned blackwinter and dr0i and unassigned blackwinter Sep 24, 2021
@blackwinter blackwinter changed the title Improve handling of XML attributes. Improve handling of XML attributes and element values. Sep 24, 2021
@@ -192,11 +211,11 @@ public void endEntity() {

@Override
public void literal(final String name, final String value) {
if (name.isEmpty()) {
if (name.isEmpty() || name.equals(valueTag)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also set DEFAULT_VALUE_TAG = "" and drop name.isEmpty().

Copy link
Member

@dr0i dr0i Sep 27, 2021

Choose a reason for hiding this comment

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

+1 simplify if you can ... did that as sideeffect of d9ba206.

Copy link
Member Author

Choose a reason for hiding this comment

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

This won't actually change the behaviour due to SimpleXmlEncoder.Element.writeElement(). But I think we should do it anyway, so we get our semantics straight.

Copy link
Member

@dr0i dr0i left a comment

Choose a reason for hiding this comment

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

Thx, I think this is very nice!
I added a commit to reuse some variables, but I am not sure if this is good, especially the direct reference of DefaultXmlPipe in SimpleXmlEncoder. You may skip the commit (as we don't have an official contributing.md yet and we are the only one working on it you may use force push).

@blackwinter blackwinter force-pushed the 379-improveHandlingOfXmlAttributes branch from d9ba206 to d6e68ff Compare September 27, 2021 21:47
@blackwinter
Copy link
Member Author

I added a commit to reuse some variables

I like the sentiment, thanks. But I see two issues with it:

  1. You included the change with the empty literal names we discussed above. Was that intentional? I actually don't think that's such a good idea. I've pulled it out into a self-contained commit.
  2. For compatibility reasons we can't set the default attribute marker for the handlers identical to the one in the encoder. That's just a minor nuisance, I guess.

as we don't have an official contributing.md yet

We don't? I thought that's what #375 was. How does it become official?

(Not that I mind, though... the rebase ban is one of my major grievances with these guidelines 😉)

@blackwinter blackwinter requested a review from dr0i September 27, 2021 21:49
@dr0i
Copy link
Member

dr0i commented Sep 28, 2021

I added a commit to reuse some variables

I like the sentiment, thanks. But I see two issues with it:

1. You included the change with the empty literal names we discussed above. Was that intentional? I actually don't think that's such a good idea. I've pulled it out into a self-contained commit.

+1 right

2. For compatibility reasons we can't set the default attribute marker for the handlers identical to the one in the encoder. That's just a minor nuisance, I guess.

ack

as we don't have an official contributing.md yet

We don't? I thought that's what #375 was. How does it become official?

ups, right! Well maybe I had slumbering in my mind that you are not so fond of it ...

(Not that I mind, though... the rebase ban is one of my major grievances with these guidelines wink)

but we don't ban it at a whole ... might you want to read again https://github.com/metafacture/metafacture-core/blob/master/CONTRIBUTING.md#force-pushing and discuss your concerns (maybe via email)?

@dr0i dr0i assigned blackwinter and unassigned dr0i Sep 28, 2021
@blackwinter blackwinter merged commit a3fd2cd into master Sep 28, 2021
@blackwinter blackwinter deleted the 379-improveHandlingOfXmlAttributes branch September 28, 2021 14:26
@dr0i dr0i mentioned this pull request Nov 2, 2021
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 this pull request may close these issues.

XML Attributes and Element values
2 participants