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
Add changing delimiters support #122
Conversation
Wow, that's fast! Again, thanks for working on this! I initially suggested changes should be made in the configuration file. Instead, you've built it using attributes. I'm going to guess you did it this way because with my proposal it would be impossible to use different syntax in different test cases within the testing crate. I think there are two ways to solve this issue:
Finally, I would like to request that you try to isolate refactoring changes to existing code (or, in fact, any change that's unrelated to the changing the delimiters) into separate commits. So ideally I'd like to have fixing the space in |
It's a deep refactoring in parser, but basically the functions, If those changes have crept in, with the IDE code refactor, I'm sorry. I have fix it. It is prepared so that in the |
Now it can be configured by file and by derive with priority in the derive. The configuration file is given the properties |
With this last change, is it similar to the idea you had? |
I still want there to be only named syntax (that is, no direct syntax manipulation via attribute fields -- only changing the syntax via an attribute field). I also still want the commit that modifies the parser macros to be split into two parts, one of which is a no-functional change refactoring. |
Now you can create named syntax in |
I can remove the modifier attributes but I would like you to explain where the problem is if it is only for test. You already had the cut, if you need anything else let me know. |
So you mentioned having more freedom for the user. I understand how that might be attractive, but as a library maintainer, more freedom for the user means less freedom for me in how to evolve the API. As such, I'd rather be more conservating about exposing API to users, such that maintaining it will be simpler over time. Additionally, I think Askama is already at the limit of how much attribute fields should be used. It's not great syntax (for example, there's less syntax highlighting at least in my editor) and having long attribute lines doesn't seem very idiomatic. So for this use case of custom syntaxes, which is arguably a bit niche, it seems more than good enough to only allow it to be done in configuration anyway. That has the added benefit to some extent of requiring users to name their syntax, which will make it more obvious what's going on if different syntaxes are used within a single crate. As for your changes, the commit introducing Could Can syntaxes be named using Again, thanks for doing this work, I know I'm a bit particular with how I want this to be done -- but as a maintainer, I'm going to be stuck with it for the foreseeable future. If you get tired of it, I can also finish it off myself (though it might take a bit more time). So thanks for working through this with me! |
Ok, I remove the attributes of more, thanks for explaining it to me. Due to the lifetimes of the I do not like clones either, but I have not found a way to call And as for The three syntaxes were to be able to compose several Syntaxes in another using the overwrite method, if the feature is removed, it remains at 2. So the main problem that I identify is the lifetime of For what I have thought, redo the last commit by removing attributes and removing the 3 Syntax. Another with the Config. And another solving the lifetime of Thank you very much for your patience, I am learning a lot with Askama. |
The If doing the other thing with TOML is hard, let's keep that for a follow-up PR to get this all straightened out first. |
The methods for Parser seem good to you? Or try the static function? I do not find another way and I see it correct as long as it is not cloned. You can use the same lifetime of the |
Oh, I'd missed that you're passing
Do you think that could work? |
Ok I like it. I will have to dismantle the Config a little to do this, it would be The only difference is to change in |
I don't understand why you want to kill |
You can not put the |
Ugh, that method stuff in nom doesn't seem to match what we're trying to do very well. Did you look at instead making a reference to the |
It is already totally static. |
Ohh, this is looking great! Using |
#121