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

StructError to include the full path #13

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

Conversation

gkoutsoumpakis-form3
Copy link

When investigating the reason that a large XML failed the validation, looking at only the NodeName might not be enough since there can be too many elements with the same name, for example address.

At the same time, while the error message contains the actual value, so it could be used to identify where the issue lies, it may not be possible to log since that might contain sensitive data.

For that reason a new field is added in StructError that contains the full path to the node that fails the validation. This is reported in reverse order for easier string concatenation. For example: child1<element1<root

When investigating the reason that a large XML failed the validation,
looking at only the NodeName might not be enough since there can be too
many elements with the same name, for example `address`.

At the same time, while the error message contains the actual value, so
it could be used to identify where the issue lies, it may not be
possible to log since that might contain sensitive data.

For that reason a new field is added in StructError that contains the
full path to the node that fails the validation. This is reported in
reverse order for easier string concatenation. For example:
`child1<element1<root`
@terminalstatic
Copy link
Owner

Hi, thanks for the contribution. While I understand the need I think that the implementation is not very flexible. My opinion is that it would be better to for example return an array of node names, something that one can use however one sees fit without manipulating a string and probably the possibility for further extending without breaking stuff. I already finished the c part for this, will keep you up do date in regard of my progress.

@gkoutsoumpakis-form3
Copy link
Author

That sounds reasonable! Looking forward to your update

@gkoutsoumpakis-form3
Copy link
Author

Hi, I just wanted to check if you have an estimate on when the changes will be ready. Is there anything I can do to help?

@terminalstatic
Copy link
Owner

terminalstatic commented Oct 25, 2022

Hi, I just wanted to check if you have an estimate on when the changes will be ready. Is there anything I can do to help?
Was on vacation for a few days and now quite busy in my day job, stalled further progress on this unfortunately. Will probably still take a few more days before I'll have the time to return to this.

@gkoutsoumpakis-form3
Copy link
Author

Hello, it's been a while since our last discussion so I thought to check again as I am still waiting for this feature to get merged. Do you think you can find some time?

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.

2 participants