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

Tag handle support #34

Closed
mrgrain opened this issue Jul 11, 2024 · 7 comments
Closed

Tag handle support #34

mrgrain opened this issue Jul 11, 2024 · 7 comments

Comments

@mrgrain
Copy link

mrgrain commented Jul 11, 2024

Hi everyone,

I ended up here following a long chain of links. Basically I am after support for Tag handles.
The approach propsed in chyh1990#135 would suit me.

This comment and this one by @davvid indicated that the respective PR made it into the davvid/yaml-rust2 fork. Subsequently it looks like this fork was abandoned in favor of this of this one (Ethiraric/yaml-rust2) - yay for consolidating!

However it appears that the specific feature didn't actually end up in here. I was wondering if there was a particular reason for it and if you would be open to a PR.

Another option could be to make the loader more composable. Right now I basically need a carbon copy of YamlLoader which my changes. But if insert_new_node were public, and every event would call a public function - then I could create a much simpler wrapper struct.

Thoughts?

@Ethiraric
Copy link
Owner

Davvid and I merged our work together. It was helped by the fact they mostly work on the api and yaml object manipulation and I on the parsing. I wasn't aware of a feature that was lost in the process.

Tags should be exposed via the low level parsing api.

I'll look this (EU) evening into more details. Especially if it changes the api.

Saphyr has an issue with a design I like for tags. This may be an option for the future.

@mrgrain
Copy link
Author

mrgrain commented Jul 11, 2024

Thanks for your response. I've started investigating as well and turns out the linked PR isn't really enough either. That only covers scalars, but tags can be used for anything!

Happy to move this discussion to saphyr. I understand now that the API here should stay the same.

@Ethiraric
Copy link
Owner

Saphyr is under development and the api may often break. You need to keep this in mind while making your choice.

Although the tags are not part of the high level api, maybe see if it's possible for you to implement an EventReceiver, which should give you access to tags.

@Ethiraric
Copy link
Owner

I'm unable to focus at the moment. I'll have to postpone my looking into this matter to tomorrow, sorry.

@Ethiraric
Copy link
Owner

Ethiraric commented Jul 12, 2024

Another option could be to make the loader more composable

Without inducing huge API changes, I don't think we can do that. Aside from copy-pasting the loader and making the changes you need, I see no option. Very little has changed in YamlLoader. Even though duplicating code is never a good option, it should be fine.

Saphyr will have proper tag handling. This is the issue I was referencing and I'll probably go for something close to what is suggested there :)

@mrgrain
Copy link
Author

mrgrain commented Jul 12, 2024

Excellent. I'll have a look at the issue in saphyr and will make sure to share my requirements. In the meantime, I'm going with the copy and modify approach of the loader.

@Ethiraric
Copy link
Owner

Heya! I'm cleaning up my notifications so I'll be closing this issue as it seems there's nothing to act upon in this repository.

Do feel free to re-open should you feel the need to! :)

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

No branches or pull requests

2 participants