You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@struts.apache.org by aleksandr-m <gi...@git.apache.org> on 2016/01/18 20:27:21 UTC
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
GitHub user aleksandr-m opened a pull request:
https://github.com/apache/struts/pull/75
WW-4590 - Allow to use multiple names in result
Allow to use multiple values in `result` tag `name` attribute and in `Result` annotation `name`.
So this typical configuration:
```
<action name="save">
<result>success.jsp</result>
<result name="error">input-form.jsp</result>
<result name="input">input-form.jsp</result>
</action>
```
Can be shorten to that:
```
<action name="save">
<result>success.jsp</result>
<result name="error, input">input-form.jsp</result>
</action>
```
And this annotations:
```
@Action(results = {
@Result(name="error", location="input-form.jsp"),
@Result(name="input", location="input-form.jsp"),
@Result(name="success", location="success.jsp")
})
```
To that:
```
@Action(results = {
@Result(name="error, input", location="input-form.jsp"),
@Result(name="success", location="success.jsp")
})
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aleksandr-m/struts feature/multiple_result_names
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/struts/pull/75.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #75
----
commit 087cf610abcd262980a41c30a2f9360aa3f416e1
Author: Aleksandr Mashchenko <al...@gmail.com>
Date: 2016-01-18T19:23:46Z
WW-4590 - Allow to use multiple names in result
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50081302
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
Why not just do it this way instead of this `if..else`?
`resultNamesSet = TextParseUtil.commaDelimitedStringToSet(resultName);`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50163270
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
No, I think why `,` is a valid result name :) But that's ok, I mean your proposal :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50221451
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
Right, thanks for explanation!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
Re: [GitHub] struts pull request: WW-4590 - Allow to use multiple
names in resu...
Posted by Chris Pratt <th...@gmail.com>.
I can't see a reason not to support both.
(*Chris*)
On Tue, Jan 19, 2016 at 12:06 PM, swiftelan <gi...@git.apache.org> wrote:
> Github user swiftelan commented on the pull request:
>
> https://github.com/apache/struts/pull/75#issuecomment-172970495
>
> @aleksandr-m
>
> An annotation attribute that is defined as an array does not have to
> have the curly braces to define a single value for that attribute.
>
> Basing a Java API for configuration on XML syntax is a poor
> implementation. Why not support the entire XML configuration syntax in the
> annotation? Java has APIs to express a collection of items. It is much
> easier to utilize an array than a comma separated value attribute. The
> array allows the developer to clearly express their intent. Utilizing an
> array does not require parsing when the configuration value is accessed.
>
>
> ---
> If your project is set up for it, you can reply to this email and have your
> reply appear on GitHub as well. If your project does not have this feature
> enabled and wishes so, or if the feature is enabled but not working, please
> contact infrastructure at infrastructure@apache.org or file a JIRA ticket
> with INFRA.
> ---
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
> For additional commands, e-mail: dev-help@struts.apache.org
>
>
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by swiftelan <gi...@git.apache.org>.
Github user swiftelan commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-172970495
@aleksandr-m
An annotation attribute that is defined as an array does not have to have the curly braces to define a single value for that attribute.
Basing a Java API for configuration on XML syntax is a poor implementation. Why not support the entire XML configuration syntax in the annotation? Java has APIs to express a collection of items. It is much easier to utilize an array than a comma separated value attribute. The array allows the developer to clearly express their intent. Utilizing an array does not require parsing when the configuration value is accessed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-172947410
@swiftelan I was thinking about that too, BUT:
* It will break backward compatibility
* I kind of like that annotation attributes looks the same as in xml configuration
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/struts/pull/75
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-173253337
Done. We should add notice in the release notes about breaking change in `ResultInfo` / `@Result` for those who use `Result.name()` or extend `ResultInfo`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-172754377
Great idea!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-173117164
@aleksandr-m I'm ok with that and I agree with @swiftelan - let's follow standards
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-172982203
@swiftelan So you are proposing to change only type? The name of the attribute will stay the same i.e `name`, right? There is still some chance that something going to break, note that `ResultInfo` depends on `Result`'s `name`. But it is better than changing the name.
@lukaszlenart WDYT about changing `Result`'s `name` to `String[] name() default com.opensymphony.xwork2.Action.SUCCESS;` and probably removing `public ResultInfo(Result, PackageConfig, String, Class<?>, Map<String, ResultTypeConfig>)` constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by swiftelan <gi...@git.apache.org>.
Github user swiftelan commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-172880314
Annotations allow multivalued attributes in the form of arrays. Using this makes it obvious that you can supply multiple result names for a single result configuration.
@Action(results = {
@Result(name={"error", "input"}, location="input-form.jsp"),
@Result(name="success", location="success.jsp")
})
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50079358
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
Currently it is allowed to have a result name as a `,`. And `TextParseUtil.commaDelimitedStringToSet` will produce empty set in that case. Note that it is trimmed so the result name can be something like " ," also.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on the pull request:
https://github.com/apache/struts/pull/75#issuecomment-173142656
I would like to merge this PR and start release process of BETA3 :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50153311
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
You mean like this?
Set<String> resultNamesSet = TextParseUtil.commaDelimitedStringToSet(resultName);
if (resultNamesSet.isEmpty()) {
resultNamesSet.add(resultName);
}
Yeah, looks a bit cleaner to me. I'll change it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50080211
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
`if (",".equals(resultName.trim()))` assumes that the result name contains only `,` and then it will produce a set with `,` (value of `resultName`). `TextParseUtil.commaDelimitedStringToSet` is in the `else` section so it won't be used in this case.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by lukaszlenart <gi...@git.apache.org>.
Github user lukaszlenart commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50076520
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
the final resuly set will contain only `,`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org
[GitHub] struts pull request: WW-4590 - Allow to use multiple names in resu...
Posted by aleksandr-m <gi...@git.apache.org>.
Github user aleksandr-m commented on a diff in the pull request:
https://github.com/apache/struts/pull/75#discussion_r50173826
--- Diff: core/src/main/java/com/opensymphony/xwork2/config/providers/XmlConfigurationProvider.java ---
@@ -777,11 +777,21 @@ protected Class verifyResultType(String className, Location loc) {
}
params.putAll(resultParams);
- ResultConfig resultConfig = new ResultConfig.Builder(resultName, resultClass)
- .addParams(params)
- .location(DomHelper.getLocationObject(element))
- .build();
- results.put(resultConfig.getName(), resultConfig);
+ Set<String> resultNamesSet;
+ if (",".equals(resultName.trim())) {
+ resultNamesSet = new HashSet<>(1);
+ resultNamesSet.add(resultName);
--- End diff --
It looks a bit odd, agree. :) But it is supported right now and it is better than mapping `,` to `success` or not creating result at all.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@struts.apache.org
For additional commands, e-mail: dev-help@struts.apache.org