Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added @XMLText decorator supporting optional 'required' option #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rzacherl
Copy link

Hi Robin,
please excuse all the unnecessary code changes inside existing files but I wasn't able to find a linter setting which preserved your code.
The code of the newly added XMLText files should be self-explanatory.

Best regards,
Robert

Copy link
Owner

@RobinBuschmann RobinBuschmann left a comment

Choose a reason for hiding this comment

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

Looks good so far 👍 Thanks for implementing. I've just added one comment and it would be good to add tests for that :)

...options
};

this.prototype.name = key;
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of this line?

@rzacherl
Copy link
Author

rzacherl commented Mar 21, 2019

I had to find a way to define the value of instance variable name without having an option name in the XMLText decorator's interface (as it makes no sense to define a name for a XML text node).
As this assignment can only be done within the annotate function (where the key argument exists), which however is a static one, I saw no other chance than to go the way via the class prototype, which gives me access to its instance variables.
The only reason why we need a name property anyhow is to be able to output the name of it within the error message shown in cases, in which the XMLText decorator defines it to be required but actually has no value assigned to it.
Maybe you see a better way to solve it.

@JavaHutt
Copy link

JavaHutt commented May 20, 2020

Maybe you see a better way to solve it.

So, guys, what's about this PR?
Attribute with text node functional would be very helpful

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

Successfully merging this pull request may close these issues.

3 participants