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

Add changing delimiters support #122

Merged
merged 3 commits into from Oct 5, 2018
Merged

Add changing delimiters support #122

merged 3 commits into from Oct 5, 2018

Conversation

zzau13
Copy link
Contributor

@zzau13 zzau13 commented Sep 27, 2018

@djc
Copy link
Owner

djc commented Sep 27, 2018

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:

  • Only test different syntaxes at the parser level (using unit tests), instead of end-to-end tests. I think this is a fine way to solve it, though it provides less flexibility to end users.
  • Use the configuration file to define named syntaxes and define a default syntax for the crate, while introducing an attribute for a different syntax on a per-template basis. This is a somewhat more involved change, but would be nicely flexible for end users.

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 Node::Call in a separate commit, moving parts of the parser into an impl in a different commit, and so on. This will make it more explicit what you are refactoring to make this change and it can be a bit more work, but it makes reviewing much easier, increasing chances that we'll be able to find any issues that come up. Let me know if all this makes sense?

@zzau13
Copy link
Contributor Author

zzau13 commented Sep 27, 2018

It's a deep refactoring in parser, but basically the functions, expr_node, cond_block, block_id, match_else_block, when_block, block_match, block_for, block_block, block_macro, block_node, block_comment, parse_template, take_content and parser to method of the new struct, Parser.

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 extract function, previously TemplateInput::new, it can pick up configuration from any side. I will implement the second option.

@zzau13
Copy link
Contributor Author

zzau13 commented Sep 27, 2018

Now it can be configured by file and by derive with priority in the derive. The configuration file is given the properties block_start, block_end, expr_start, expr_end, comment_start, comment_end of 2 characters and the developer has to take care of the combinations of the different "start" braces because the method take_content.

@zzau13
Copy link
Contributor Author

zzau13 commented Sep 29, 2018

With this last change, is it similar to the idea you had?

@djc
Copy link
Owner

djc commented Sep 29, 2018

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.

@djc djc closed this Sep 29, 2018
@djc djc reopened this Sep 29, 2018
@zzau13
Copy link
Contributor Author

zzau13 commented Sep 29, 2018

Now you can create named syntax in askama.toml, and add his name to the default_syntax attribute without any modifier attribute (block_start,..). Modifiers by attribute in addition to this. You can create several and combine them, overwriting them in the attributes or changing them from the attribute with what allows me to test my code. And still have the behavior you described just a little more free for the user. It is not necessary to use it and not to behave any problem.
Ok, now I divide the first commit in two,

@zzau13 zzau13 closed this Sep 29, 2018
@zzau13 zzau13 reopened this Sep 30, 2018
@zzau13
Copy link
Contributor Author

zzau13 commented Sep 30, 2018

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.

@djc
Copy link
Owner

djc commented Sep 30, 2018

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 Parser now changes in the TemplateInput type to support parser creation, but with this direction, that should likely be rolled back, with syntax just becoming a field on TemplateInput.

Could Parser::parse() take a reference instead of an owned self? Cloning the Parser all over the places seems a little ugly.

Can syntaxes be named using [[syntax.foo]] rather than having an option name inside the syntax block? I think something like that is probably possible with TOML, but I haven't looked deeply into it. I'd prefer not to have a separate default_syntax on Config, it can just be named entry "default" in the syntaxes map. It also seems like we shouldn't need a Syntax, an OptionSyntax and a RawSyntax, although I can see how we might need two rather than three.

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!

@zzau13
Copy link
Contributor Author

zzau13 commented Sep 30, 2018

Ok, I remove the attributes of more, thanks for explaining it to me.

Due to the lifetimes of the TemplateInput properties, it is not possible to copy or clone, since it lives in the heap and the stack at the same time. So it can not be used as an argument of Parser::parse()

I do not like clones either, but I have not found a way to call named_args! inside the macro alt!, many0!, etc and even if I did it I would have to clone or copy the Syntax struct. Clone could be avoided by a copy if I change the lifetimes, but now they came in the heap from here. And the main problem with the current implementation of the attributes is that they do not allow to set the lifetime in the stack.
And I was also thinking that we could pair other languages apart from Jinja, with few changes from this struct.

And as for [[syntax.name]], it can be done, but it's a big change in Config::from_str, looking for these options by discard in the Table, BTreeMap <String, Value>. I add the test with the feature.

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 TemplateInput and is a deep refactor. I can put it after these commits, I was already thinking how.

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 TemplateInput and all those ugly clones.
How about?

Thank you very much for your patience, I am learning a lot with Askama.

@djc
Copy link
Owner

djc commented Sep 30, 2018

The TemplateInput should live long enough, though, to just pass it as a reference to everything else? Also ideally you don't pass the full TemplateInput to the parser, but instead pass (a reference to) the relevant Syntax instance from the Config keyed with the attribute field's syntax key (which should also live plenty long enough). Does that make sense?

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.

@zzau13
Copy link
Contributor Author

zzau13 commented Sep 30, 2018

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 build_template function for everything else, including TemplateInput. In other words, nothing apart from the return of build_template must continue to exist. It can be done, but the refactor is deep, I do it?

@djc
Copy link
Owner

djc commented Oct 1, 2018

Oh, I'd missed that you're passing mut self to the Parser methods. Is that why you're cloning it around? I think in this case you should be able to just use &self. If you do that, I don't see why you'd need deep refactoring. What I want is kind of like this:

  • Restore TemplateInput::new(), TemplateInput will include a new syntax: Option<&str> field
  • build_template() gets a &Syntax out of the Config::syntaxes map based on input.syntax
  • Parser::new() takes a &Syntax
  • Parser methods take &self
  • generator::generate() also takes a &Syntax

Do you think that could work?

@zzau13
Copy link
Contributor Author

zzau13 commented Oct 1, 2018

Ok I like it.

I will have to dismantle the Config a little to do this, it would be Config::new(& str), remove Config::from_str and the logic of the fs::read_to_string will draw it to TemplateInput::new. So the config disappears from TemplateInput in favor of dirs: Vec<PathBuf> and parser: &'a Parser.

The only difference is to change in TemplateInput, config for dirs and let that config die. The idea is to eliminate the config that we do not need and deserialize without alloc. How about?

@djc
Copy link
Owner

djc commented Oct 1, 2018

I don't understand why you want to kill Config or why you'd have to. I do think TemplateInput::new() instantiating the Config is a little bit off, and it would be better to move that into build_template(), which would then pass &Config into TemplateInput::new()?

@zzau13
Copy link
Contributor Author

zzau13 commented Oct 1, 2018

You can not put the "&" in the self field of the macro method!. The only way to use call_m! more than once in the same method is to use mut self https://docs.rs/nom/4.0.0/nom/methods/index.html.
Now I'll do the refactor for TemplateInput::new(ast, &config).

@djc
Copy link
Owner

djc commented Oct 2, 2018

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 Syntax part of the Input type, instead?

@zzau13
Copy link
Contributor Author

zzau13 commented Oct 4, 2018

It is already totally static.

@djc
Copy link
Owner

djc commented Oct 5, 2018

Ohh, this is looking great! Using named_args!() for the parser seems like a very nice solution. I'm going to merge this now and tweak some things just a little bit.

@djc djc merged commit 52bdd4b into djc:master Oct 5, 2018
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.

None yet

2 participants