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

Adding Dynamic Names #1

Merged
merged 10 commits into from Sep 17, 2022
Merged

Adding Dynamic Names #1

merged 10 commits into from Sep 17, 2022

Conversation

ghost
Copy link

@ghost ghost commented Aug 15, 2022

I need a review, let me know if this is okay and if I need to build this with Ronn.
I will be pretty busy these next few days until September so feel free to commit changes in my fork by yourself if you feel to. I will try my best to be here. :)

I need a review, let me know if this is okay and if I need to build this.
Copy link
Owner

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Sorry for the delayed review, for some reason I was not watching all activity on my own fork yet. That's fixed now.

Besides my comments below, I have one request:

  • There is a paragraph near the top of the manpage where it mentions the spec. That should be updated to say that it reflects version 1.3.0 of the spec.

and one suggestion:

  • Maybe briefly mention dynamic names again in the section on Parent tags. Compare to how I document dotted names in the Variables section, and then briefly mention them again in the Blocks section.

I need a review, let me know if this is okay and if I need to build this with Ronn.

It's convenient for me if you build it, but I know this can be challenging. If it proves too challenging, I don't mind doing it myself.

I will be pretty busy these next few days until September so feel free to commit changes in my fork by yourself if you feel to. I will try my best to be here. :)

I might do that at some point, if I think I can make the changes much sooner than you (which is not yet the case), but for now, I'll just leave it at documenting my thoughts. :-)

man/mustache.5.ron Outdated Show resolved Hide resolved
man/mustache.5.ron Outdated Show resolved Hide resolved
man/mustache.5.ron Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Sep 1, 2022

Thanks a lot! Sorry for the delayed review, for some reason I was not watching all activity on my own fork yet. That's fixed now.

No worries. I have been kind of busy lately but now I'm back at it again!

Besides my comments below, I have one request:

* There is a paragraph near the top of the manpage where it mentions the spec. That should be updated to say that it reflects version 1.3.0 of the spec.

and one suggestion:

* Maybe briefly mention dynamic names again in the section on Parent tags. Compare to how I document dotted names in the Variables section, and then briefly mention them again in the Blocks section.

I got it. I am working on these right now.

anomal00us and others added 2 commits September 1, 2022 21:01
Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>
@ghost
Copy link
Author

ghost commented Sep 2, 2022

Okay I tried to make the description less technical and verbose.
I am not sure whether the way I mentioned Dynamic Names in the Blocks and Parents paragraphs is okay, what was your idea on that?

Copy link
Owner

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether the way I mentioned Dynamic Names in the Blocks and Parents paragraphs is okay, what was your idea on that?

Your current text seems to be based on the observation that inheritance and dynamic partials are to some extent alternatives. Since most manpage readers will be unaware of that, I agree it's probably not very helpful.

I meant that some implementations (at least mine, possibly also the ones by @gasche or @bobthecow) allow using dynamic names inside parent tags:

{{<*path.to.parentName}}{{/*path.to.parentName}}

So it may be useful to mention that this is an option in some implementations, similar to how the Section section mentions that dotted names are an option (after the Variables section has already explained at length what dotted names do).

man/mustache.5.ron Outdated Show resolved Hide resolved
Comment on lines 420 to 422
Dynamic Names consists of an asterisk, followed by a dotted name which follows
the same notation and the same resolution as in an Interpolation tag. That is
`{{>*dynamic}}`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps compare this to the hypothetical {{>{{dynamic}}}} to further clarify what it does.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that including wrong examples may be confusing

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are quite right that it could be confusing. Very friendly of you to go with it anyway! I still think the hypothetical nested tag can also serve to clarify the concept, but please feel absolutely welcome to add an additional remark, something like (which is not allowed!) just before or after the forbidden example.

man/mustache.5.ron Outdated Show resolved Hide resolved
man/mustache.5.ron Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Sep 15, 2022

Okay I'm back at it again!
Sorry for the delay I was emotionally struggling lol
Now it's time to work 💪
Pushing some changes soon

@jgonggrijp
Copy link
Owner

Sorry for the delay I was emotionally struggling lol

Oh, no need to apologize! I hope the struggle was not because of this pull request? In any case, best wishes!

(I will review next.)

Copy link
Owner

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite happy with this! At this point, I feel I'm just nitpicking on details.

man/mustache.5.ron Outdated Show resolved Hide resolved
Comment on lines 420 to 422
Dynamic Names consists of an asterisk, followed by a dotted name which follows
the same notation and the same resolution as in an Interpolation tag. That is
`{{>*dynamic}}`.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are quite right that it could be confusing. Very friendly of you to go with it anyway! I still think the hypothetical nested tag can also serve to clarify the concept, but please feel absolutely welcome to add an additional remark, something like (which is not allowed!) just before or after the forbidden example.

man/mustache.5.ron Outdated Show resolved Hide resolved
man/mustache.5.ron Outdated Show resolved Hide resolved
anomal00us and others added 3 commits September 16, 2022 00:02
Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>
Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>
Co-authored-by: Julian Gonggrijp <dev@juliangonggrijp.com>
@ghost
Copy link
Author

ghost commented Sep 15, 2022

Oh, no need to apologize! I hope the struggle was not because of this pull request? In any case, best wishes!

Noooo noo absolutely not! I like working with you!
It was due a girl.
Ehh.. such is love xD
But I feel better now, thank you

Okay so now I have committed all the changes you suggested and the only thing I have left is to add an additional remark to that forbidden example we were discussing earlier.
I'm committing in a few minutes and then I think I'm done!

Copy link
Owner

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, thanks! Feel free to compile the thing, or not. Otherwise I'll do it just before merging (that will be tomorrow at the earliest, I'm going to bed now).

@ghost
Copy link
Author

ghost commented Sep 15, 2022

Great job, thanks! Feel free to compile the thing, or not. Otherwise I'll do it just before merging (that will be tomorrow at the earliest, I'm going to bed now).

Awesome!
Okay I managed to compile the thing, you should take a look at my last commit, if something went wrong I can revert it.
PS. I had to edit the Rakefile too since it was looking for *.ronn files meanwhile we have *.ron.
I'm also going to bed now, goodnight! 😴

@bobthecow
Copy link

I meant that some implementations (at least mine, possibly also the ones by gasche or bobthecow) allow using dynamic names inside parent tags

Yup, my implementation does as well.

@jgonggrijp
Copy link
Owner

@anomal00us could you change the last commit so it omits the mustache.1 files? The annoying thing about the rebuild is that it makes those files seem as if they were updated on the date of the latest build, even though the content is exactly the same as... many years ago.

@ghost
Copy link
Author

ghost commented Sep 17, 2022

@anomal00us could you change the last commit so it omits the mustache.1 files? The annoying thing about the rebuild is that it makes those files seem as if they were updated on the date of the latest build, even though the content is exactly the same as... many years ago.

Sure!

@jgonggrijp
Copy link
Owner

Thanks again @anomal00us !

@jgonggrijp jgonggrijp merged commit 90ca004 into jgonggrijp:manpage-update-spec Sep 17, 2022
@ghost
Copy link
Author

ghost commented Sep 17, 2022

You're welcome, it has been nice to work with you :)

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.

2 participants