[GitHub] ant-ivy pull request #53: Suppressed warnings

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
GitHub user twogee opened a pull request:

    https://github.com/apache/ant-ivy/pull/53

    Suppressed warnings

    As described in previous PR, all compiler warnings are annotated.
    This PR removes a few of them, along with some refactoring.
    Changes are grouped in separate commits which are hopefully self-describing.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/twogee/ant-ivy suppressed-warnings

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/ant-ivy/pull/53.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 #53
   
----
commit 3781faf7238a3a9dc185a863f8b0582a7c28d1b4
Author: twogee <[hidden email]>
Date:   2017-07-18T12:08:13Z

    cosmetics

commit ba221a1b4323aa59137886b48c70d2ba3480325f
Author: twogee <[hidden email]>
Date:   2017-07-18T12:09:12Z

    get rid of suppressed warnings

commit bd0f075d2cecf7d6d86c9ce92ed6727438f06f05
Author: twogee <[hidden email]>
Date:   2017-07-18T12:09:52Z

    refactor

commit 2f0cd05b6ab4bb84eff3b21ebbc966de23e740f0
Author: twogee <[hidden email]>
Date:   2017-07-18T12:10:08Z

    fix exceptions

----


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    Hi Gintas,
   
    Given the amount of commits and the changes involved in this PR, it's a bit difficult to review it. I think this PR even includes some of the changes that have already been included in upstream master from one of your other PR.
   
    What I have instead now done is - reviewed individual smaller commits from this PR which were isolated enough to be easily reviewed/merged.
   
    I have `cherry-pick`ed the following commits from this PR, squashed them in one commit and merged it upstream:
   
    - https://github.com/apache/ant-ivy/pull/53/commits/cceb05ebfce3d481a97e46f384afbcd2eca66005
   
    - https://github.com/apache/ant-ivy/pull/53/commits/cce7276a93e50c7ed608987697ec08d3b0d2f2af
   
    - https://github.com/apache/ant-ivy/pull/53/commits/381991072529d832fcdfb422d2e99599d037d30e
   
    The merged commit upstream is this one https://github.com/apache/ant-ivy/commit/d4c3cbccaf05beeb11712294ef4dd0af25d9d0f4
   
    Furthermore, since the same PR keeps getting updated regularly with changes and new commits, it becomes difficult to keep track of what to review and what's pending.
   
    For the rest of the commits that I haven't been able to review yet (given their size and nature), could you please create isolated individual PRs (on separate personal branches), all rebased against latest master branch? That will make it a bit more easier to review them.



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    Sorry for additions to PR. I figured out a way to remove the last inconsistencies with generics without breaking the API. This PR will not change any more. I rebased my commits; you're welcome to cherrypick.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129593699
 
    --- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
    @@ -71,132 +71,139 @@
     
    --- End diff --
   
    This file has a large change which appears to be reordering/restructuring the code logic. Given the nature of this diff, it's difficult to review it and see if this change introduces any changes in the logic or introduces any regressions. Plus this class is one of the very core ones. Also, I'm not too sure if the existing test cases cover for this level of change.
   
    Given all this, I'm not confident that these changes in this specific class are worth it, at least at the moment. Unless of course, some one else can review this specific class and approve it, would you be willing to undo the changes to this one class in this PR?



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129587968
 
    --- Diff: src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java ---
    @@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, LatestStrategy strategy) {
             if (versionMatcher.isDynamic(mrid)) {
                 while (iter.hasNext()) {
                     IvyNode other = iter.next();
    -                if (versionMatcher.isDynamic(other.getResolvedId())) {
    +                if (versionMatcher.isDynamic(other.getResolvedId())
    +                        || !versionMatcher.accept(mrid, other.getResolvedId())
    +                        && !handleIncompatibleConflict(parent, conflicts, node, other)) {
    --- End diff --
   
    This change doesn't look right. The previous logic would have returned `null` if the `versionMatcher.isDynamic` returned true. With this change, it will require an additional `&&` to be satisfied for the `!handlerIncompatibleConflict...`


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    Commit https://github.com/apache/ant-ivy/pull/53/commits/432c42ad1ee54ab08ea8b8808086ba19d4880737 in this PR has now been merged upstream https://github.com/apache/ant-ivy/commit/080e96a6cdea2f5fe8727b8249044b89bc54395d


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129660986
 
    --- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
    @@ -71,132 +71,139 @@
     
    --- End diff --
   
    It's a change from if ... else if ... else if ... else if ... to a switch, which changes indentation and messes up git diff. I will rebase and rearrange the remaining commits.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129663139
 
    --- Diff: src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java ---
    @@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, LatestStrategy strategy) {
             if (versionMatcher.isDynamic(mrid)) {
                 while (iter.hasNext()) {
                     IvyNode other = iter.next();
    -                if (versionMatcher.isDynamic(other.getResolvedId())) {
    +                if (versionMatcher.isDynamic(other.getResolvedId())
    +                        || !versionMatcher.accept(mrid, other.getResolvedId())
    +                        && !handleIncompatibleConflict(parent, conflicts, node, other)) {
    --- End diff --
   
    [It still does](http://introcs.cs.princeton.edu/java/11precedence/). There's a difference between `&&` and `||` vs `&` and `|`, see "short circuiting".


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129668076
 
    --- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
    @@ -71,132 +71,139 @@
     
    --- End diff --
   
    Here's a "diff -wc"-style diff
    [XmlReporteParser.zip](https://github.com/apache/ant-ivy/files/1177761/XmlReporteParser.zip)



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    This is strange, all tests still pass for me locally -- could someone elucidate what the problem is, please?


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    D'uh... getExtends() must respect insertion order -- array straitjacket 😄


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    I'm sorry for not explaining the point with all the changes around foreach loops: they take anything that is iterable rather than an array (which Ivy is very fond of API-wise) and by doing so allow for API changes where a collection is a more natural choice (vis-à-vis streams in Java 8+). I was first afraid that a change of API might be necessary sooner rather than later (because of inconsistencies in generics). That is avoided for now, but should the question be raised again, this PR tries to lay a groundwork indicating a direction of a preferred change.


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy pull request #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user jaikiran commented on a diff in the pull request:

    https://github.com/apache/ant-ivy/pull/53#discussion_r129757849
 
    --- Diff: src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java ---
    @@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, LatestStrategy strategy) {
             if (versionMatcher.isDynamic(mrid)) {
                 while (iter.hasNext()) {
                     IvyNode other = iter.next();
    -                if (versionMatcher.isDynamic(other.getResolvedId())) {
    +                if (versionMatcher.isDynamic(other.getResolvedId())
    +                        || !versionMatcher.accept(mrid, other.getResolvedId())
    +                        && !handleIncompatibleConflict(parent, conflicts, node, other)) {
    --- End diff --
   
    You are right. Although I was aware of short circuiting, I had a wrong semantic in mind about it. I was under the impression, the short circuiting happens "smartly" depending on what other operators are present in that expression. I read up the JLS (section 15.24)[1] now and as you note, the right side of the `||` will be completely ignored (irrespective of what additional operators exist in that expression) if the left side returns true. So this change of the `if` block is fine.
   
    [1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.24


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user jaikiran commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    Commit https://github.com/apache/ant-ivy/pull/53/commits/414651c8841a09a9d9d4b0365ee8e7227fba8429 in this PR has now been merged to upstream https://github.com/apache/ant-ivy/commit/1a283ab1be5b495f5be8cf6c2ab2b96453dd32f9
   
    It had a merge conflict against latest upstream master which I had to resolve during the review/merge.
   
    At this point, the only remaining commit that needs review in this PR is this one https://github.com/apache/ant-ivy/pull/53/commits/b7c26ac4ea31369e7c0945aea8552280857f951a. Given that this hasn't been rebased against latest master branch, can you please close this PR and reopen a new one containing this one single commit, rebased against latest master. That commit includes changes to many files, so having it rebased against latest master will help review it properly.



---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] ant-ivy issue #53: Suppressed warnings

apupier
In reply to this post by apupier
Github user twogee commented on the issue:

    https://github.com/apache/ant-ivy/pull/53
 
    With all due respect, there are some refactorings worth attention IMHO. Besides, some deprecations need to be followed through 😉


---
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 [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...