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 wml_actions.make_tag as proposed at #1503 #931

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@gfgtdf
Contributor

gfgtdf commented Feb 12, 2017

No description provided.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Feb 12, 2017

Member

Okay, two things.

  1. I'm not entirely happy with your choice of names; why [wml_defined_tag] instead of [make_tag] for example? And notice that [insert_tag] uses name rather than tagname, so it might be good for [make_tag] to do the same.

  2. What if the new tag calls itself? With the current implementation that would lead to a stack overflow, I think. It would be possible to prevent that and at the same time make it possible for the tag to extend an existing tag.

If you want, I could address point 2.

By the way, will there be a way to remove a tag created with this? Also, what happens if some Lua overrides the tag that was created with this tag?

Member

CelticMinstrel commented Feb 12, 2017

Okay, two things.

  1. I'm not entirely happy with your choice of names; why [wml_defined_tag] instead of [make_tag] for example? And notice that [insert_tag] uses name rather than tagname, so it might be good for [make_tag] to do the same.

  2. What if the new tag calls itself? With the current implementation that would lead to a stack overflow, I think. It would be possible to prevent that and at the same time make it possible for the tag to extend an existing tag.

If you want, I could address point 2.

By the way, will there be a way to remove a tag created with this? Also, what happens if some Lua overrides the tag that was created with this tag?

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Feb 12, 2017

Contributor

I'm not entirely happy with your choice of names; why [wml_defined_tag] instead of [make_tag] for example?

Well that [wml_defined_tag] is only used for the internal savefile storage the actionswml tag is make_tag

What if the new tag calls itself

Hmm yes i don't think this is a searious issue, we already have this situation in every other progrmming language, it is even quite possible that lua does give you a proper erro message in this case.

By the way, will there be a way to remove a tag created with this

no

Also, what happens if some Lua overrides the tag that was created with this tag?

well i don't think is something we need to support (defining behviour).

Contributor

gfgtdf commented Feb 12, 2017

I'm not entirely happy with your choice of names; why [wml_defined_tag] instead of [make_tag] for example?

Well that [wml_defined_tag] is only used for the internal savefile storage the actionswml tag is make_tag

What if the new tag calls itself

Hmm yes i don't think this is a searious issue, we already have this situation in every other progrmming language, it is even quite possible that lua does give you a proper erro message in this case.

By the way, will there be a way to remove a tag created with this

no

Also, what happens if some Lua overrides the tag that was created with this tag?

well i don't think is something we need to support (defining behviour).

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Feb 12, 2017

Contributor

whta currently bothers me more is that: The the [a] variables tag which contains the argumnts passed to the tag can easily be used as a continer for 'local' variables since it gets automatically deleted after the tag. I do like this idea but it also has a flaw: If your custom tag calls another custom tag that stores something you to a varible sepcified in a [yourtag2] variable= key, you cannt specify variable=a.res since [a] is deleted after the inner tag returns. Maybe we should add some advanced 'return value' mechanic to this.

Contributor

gfgtdf commented Feb 12, 2017

whta currently bothers me more is that: The the [a] variables tag which contains the argumnts passed to the tag can easily be used as a continer for 'local' variables since it gets automatically deleted after the tag. I do like this idea but it also has a flaw: If your custom tag calls another custom tag that stores something you to a varible sepcified in a [yourtag2] variable= key, you cannt specify variable=a.res since [a] is deleted after the inner tag returns. Maybe we should add some advanced 'return value' mechanic to this.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Feb 12, 2017

Member

Oh wow, I didn't even notice that. [a] is an absolutely terrible tag name. It should definitely be something like [args] for consistency with other tags (eg the [lua] tag). And what about changing the tagname attribute for consistency too?

I don't really understand your actual concern over the args tag, though...

Also, what happens if some Lua overrides the tag that was created with this tag?
well i don't think is something we need to support (defining behviour).

There probably doesn't need to be direct support for it, but... I think it's clear that this could be a source of bugs. The reverse could also be true - cases where Lua overrides the tag before this tag replaces it.

What if the new tag calls itself
Hmm yes i don't think this is a searious issue, we already have this situation in every other progrmming language, it is even quite possible that lua does give you a proper erro message in this case.

It's probably not a really serious issue, but I think it might be good to support the concept of extending the tag, rather than replacing it, which works to eliminate this issue.

Well that [wml_defined_tag] is only used for the internal savefile storage the actionswml tag is make_tag

True, but there's no reason to use nonsensical tag names just because it's for an internal use. (Admittedly, this isn't exactly a nonsensical tag name, though.)

Member

CelticMinstrel commented Feb 12, 2017

Oh wow, I didn't even notice that. [a] is an absolutely terrible tag name. It should definitely be something like [args] for consistency with other tags (eg the [lua] tag). And what about changing the tagname attribute for consistency too?

I don't really understand your actual concern over the args tag, though...

Also, what happens if some Lua overrides the tag that was created with this tag?
well i don't think is something we need to support (defining behviour).

There probably doesn't need to be direct support for it, but... I think it's clear that this could be a source of bugs. The reverse could also be true - cases where Lua overrides the tag before this tag replaces it.

What if the new tag calls itself
Hmm yes i don't think this is a searious issue, we already have this situation in every other progrmming language, it is even quite possible that lua does give you a proper erro message in this case.

It's probably not a really serious issue, but I think it might be good to support the concept of extending the tag, rather than replacing it, which works to eliminate this issue.

Well that [wml_defined_tag] is only used for the internal savefile storage the actionswml tag is make_tag

True, but there's no reason to use nonsensical tag names just because it's for an internal use. (Admittedly, this isn't exactly a nonsensical tag name, though.)

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Feb 13, 2017

Contributor

Hmm i currently forgot what is the advantage of this is over the usualy [fire_event] which somehow decreases my motivation to work on this. Will try to remember.

Contributor

gfgtdf commented Feb 13, 2017

Hmm i currently forgot what is the advantage of this is over the usualy [fire_event] which somehow decreases my motivation to work on this. Will try to remember.

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Feb 16, 2017

Member

For the record, I've never particularly thought this was a great idea.

Member

Vultraz commented Feb 16, 2017

For the record, I've never particularly thought this was a great idea.

@sapientN3T

This comment has been minimized.

Show comment
Hide comment
@sapientN3T

sapientN3T Feb 25, 2017

Contributor

The name "make_tag" is deceptive, since you are not able to make new tags such as conditional tags, for example. Better to call it a function ("set_function") which should support arguments "args" and a return structure, as well as a way to specify the variable that will ultimately be replaced/merged/appended with the return structure. Also, it could be called via "call_function"... although the idea of overwriting tags may sound alluring to some WML coders, that might cause more harm than good.

Contributor

sapientN3T commented Feb 25, 2017

The name "make_tag" is deceptive, since you are not able to make new tags such as conditional tags, for example. Better to call it a function ("set_function") which should support arguments "args" and a return structure, as well as a way to specify the variable that will ultimately be replaced/merged/appended with the return structure. Also, it could be called via "call_function"... although the idea of overwriting tags may sound alluring to some WML coders, that might cause more harm than good.

@sigurdfdragon

This comment has been minimized.

Show comment
Hide comment
@sigurdfdragon

sigurdfdragon Mar 18, 2017

Contributor

Since one can already make new action and conditional tags in Lua,
It seems the main/only benefit of this would be to allow people who wish to define new action tags in wml to do so.

Contributor

sigurdfdragon commented Mar 18, 2017

Since one can already make new action and conditional tags in Lua,
It seems the main/only benefit of this would be to allow people who wish to define new action tags in wml to do so.

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Mar 18, 2017

Contributor

to allow people who wish to define new action tags in wml to do so.

Yes this is exactly the intantion of this patch.

Contributor

gfgtdf commented Mar 18, 2017

to allow people who wish to define new action tags in wml to do so.

Yes this is exactly the intantion of this patch.

@Wedge009

This comment has been minimized.

Show comment
Hide comment
@Wedge009

Wedge009 May 9, 2017

Member

GNA #23841 is now Issue #1503.

Member

Wedge009 commented May 9, 2017

GNA #23841 is now Issue #1503.

@Wedge009 Wedge009 changed the title from add wml_actions.make_tag as proposed at https://gna.org/bugs/?23841 to add wml_actions.make_tag as proposed at #1503 May 9, 2017

@Vultraz

This comment has been minimized.

Show comment
Hide comment
@Vultraz

Vultraz Nov 1, 2017

Member

Honestly, I don't feel this is the best way to go, feature-wise. Closing.

Member

Vultraz commented Nov 1, 2017

Honestly, I don't feel this is the best way to go, feature-wise. Closing.

@Vultraz Vultraz closed this Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment