-
Notifications
You must be signed in to change notification settings - Fork 94
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
Make 'special behavior characters' configurable and escapable #63
Comments
This is directly related to #55 |
I think this is a good idea, and the best approach to addressing the issue of backwards compatibility with existing datasets as well. While I think making these prefixes configurable addresses most of the problem, I still think it would be ideal to provide the ability to escape special characters as well. I don't think escaping special characters is an absolute necessity though if we can make the special prefixes configurable.
Not sure I follow the worry about yaml here; since it's our code that interprets the prefix characters, it's our code that would interpret the escape characters as well rather than the yaml parser |
I am working on a solution for the issues mentioned here - so far it is only covering the string interpolation, but that was actually the hardest part to implement as the very limited parser would not allow for such changes. So I have rewritten the RefValue class from scratch, offering these features:
The interpolation sentinels remain configurable and the escape character or string (supports multiple characters) can be configured too. This would cover most of the problems that were discussed in this issue. Also, the configurable escape character could be really easy built into the application list the same way (escape and double-escape, if you want the literal escape character itself) - and would not even need a parser, as str.startswith() should be sufficient. Would love to hear some feedback. I do know that docblocks are not yet updated, formatting might not be perfect, error handling is not perfect yet and there are zero written tests, but if I can get this somehow upstream merged, I will gladly polish it all and send in a PR. My implementation can be found at NeoXiD/reclass in the feature/escaping branch - alternatively, here is the link to the commit implementing that: Click me EDIT: Forgot to mention, this is especially relevant for issue #55 |
This would definitely be my preferred approach to solve this problem. Moreover, I do not have a great deal of experience writing parsers, so this contribution would, at least in my case, save a rather time intensive learning curve. I would like to see a PR from your feature/escaping branch into master where myself and @Rtzq0 could more formally review this change and offer more detailed feedback. |
RE: the conversation around #61 , it seems like a good idea to attack the problem of the fact that the application negation prefix is hardcoded.
Furthermore, even when we make it configurable, there remains the issue of needing some way to escape these "special prefix" characters.
This is easy enough to implement but yaml is not big on "escaping" things so I'm not sure how to do this idiomatically. I think the solution that would appear obvious to most users is
/
but I'd appreciate feedback on that.An alternative (although much much more involved approach) would be to alter the way the filesystem backend works such that
~application
s subclass scalars and~keys
subclass keys.on reflection it also seems clear that if we make all of these configurable, although it's not ideal, we could wontfix this issue an have there be no escape sequence on the grounds that there's plenty enough space on the keyboard to pick something that is still a valid hash key
The text was updated successfully, but these errors were encountered: