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

WW-4888 add escaping possibilities to text-tag #181

Merged
merged 2 commits into from
Nov 9, 2017
Merged

WW-4888 add escaping possibilities to text-tag #181

merged 2 commits into from
Nov 9, 2017

Conversation

sdutry
Copy link
Member

@sdutry sdutry commented Nov 8, 2017

Added the option of escaping the result of an <s:text> tag.

Defaults set to not escaping in order not to break current behavior.

possible improvement:

  • now holds duplicate prepare method from the Property class

@sdutry sdutry requested a review from lukaszlenart November 8, 2017 22:16
@lukaszlenart lukaszlenart self-assigned this Nov 9, 2017
@yasserzamani
Copy link
Member

Maybe we can ask user to use PropertyTag instead for such cases.

Or if we would like to add this also to TextTag , I would refactor this into another super tag and then inherit it in both PropertyTag and TextTag :)

@lukaszlenart
Copy link
Member

PropertyTag shouldn't be used to display messages and I have no issue with having these properties duplicated

@yasserzamani
Copy link
Member

So looks good; I could not fuss about any thing at first ;)

@lukaszlenart
Copy link
Member

ok, LGTM 👍

@lukaszlenart lukaszlenart merged commit 0fb84fa into apache:master Nov 9, 2017
@sdutry sdutry deleted the WW-4888 branch November 9, 2017 17:32
@aleksandr-m
Copy link
Contributor

And we can easily change false to true in 2.6.

@yasserzamani
Copy link
Member

As we removed snippets in docs, Parameters section of /source/tag-developers/text-tag.md should being updated also.

@sdutry
Copy link
Member Author

sdutry commented Nov 15, 2017

As we removed snippets in docs, Parameters section of /source/tag-developers/text-tag.md should being updated also.

@yasserzamani
I see you updated the docs.
Personaly i would have waited till the official release of the new struts version.
Changing it automaticaly updates the struts site, which now references a non-officialy released struts version.

Personaly I don't think its a problem for this specific case, it's just something to take into account when updating docs for upcoming versions..

Maybe a possible solution to prevent this in the future could be to have a seperate branch for the documentation updates for the new release that gets merged when it gets officialy released?

What do you think @lukaszlenart ?

@yasserzamani
Copy link
Member

Parameters section of /source/tag-developers/text-tag.md should being updated also.

Done! To test my first direct commit as a Committer 😉

@yasserzamani
Copy link
Member

Oh I'm sorry :( I thought you did not find a time and I just want to test if I can commit as a Committer and found this is a good case.

I was sure because about one month ago, I already have an accepted merge at WW-4034 Adds docs for how customizing JSONWriter.

IMHO, maybe it is not bad users know what comings up 😉 I think new branches may add complexities and release needed works.

@lukaszlenart
Copy link
Member

@sdutry an another branch is a nice idea, I think this would work.

Anyway, as we do not provide a separated copy of documentation per each release (as it used to be), we have to be sure that we're adding a since tag to each new option.

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.

4 participants