-
Notifications
You must be signed in to change notification settings - Fork 8
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
Simplify unexpected elements code #15
Conversation
- Remove unnecessary references to options.namedTypes - Remove unneeded entry in .gitignore
This looks cleaner for sure and is closer to my original approach but I was afraid you’d prefer that I operate within the pattern you’d already developed. I’m happy with this. I can merge this in to #14 like you suggested. Just let me know when you’re ready. |
Thanks for the feedback. It needs a bit more work however - I accidentally disabled too many tests in this PR. Let me push the current state. |
0e3d989
to
dc75416
Compare
The current state (as of commit dc75416) isn't quite right yet. It's not using the top-level types correctly, as can be seen in the failing tests.
Thank you for your consideration! In this case, the existing pattern is to store elements observed in the input XML documents. For elements that are not observed then it's OK to use some kind of special case/side channel code. This also avoids potential conflicts if the element names overlap. BTW, I see that #14 is your first PR on GitHub. It's a good PR - thank you! For issues like this - where there are multiple design options that might fit better or worse into the existing structure - don't hesitate to follow Dave Cheney's advice of talk, then code. Happy to hand this back to you if you'd like to take it from here. |
Thanks for the observations and the pointers. I’m open to any help you want to provide. This is a nice clean library and I’m learning a bit about golang best practices here too. I’ve got a more storied history with Java and Python but I’m a relative newcomer to golang so this is valuable experience. Where ever you want me to pick it up, I can. I won’t mess with it anymore today but I can take a look tomorrow. I’ll merge whatever you have in #15 in to #14 and take care of anything outstanding from there, if that works for you. On Nov 28, 2023, at 15:29, Tom Payne ***@***.***> wrote:
The current state (as of commit dc75416) isn't quite right yet. It's not using the top-level types correctly, as can be seen in the failing tests.
I was afraid you’d prefer that I operate within the pattern you’d already developed.
Thank you for your consideration! In this case, the existing pattern is to store elements observed in the input XML documents. For elements that are not observed then it's OK to use some kind of special case/side channel code. This also avoids potential conflicts if the element names overlap.
BTW, I see that #14 is your first PR on GitHub. It's a good PR - thank you! For issues like this - where there are multiple design options that might fit better or worse into the existing structure - don't hesitate to follow Dave Cheney's advice of talk, then code.
Happy to hand this back to you if you'd like to take it from here.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Over to you! No rush on anything. I'll close this PR so you can work on #14 in your own time. |
Refs #14.
cc @cbrow111
Please look at commit 0e3d989 for the proposed simplification of the code. There are a couple more changes that I'd like to add.
If you're happy with this, then I propose that we combine this into #14 to ensure that you get the author credit for the feature.