activemq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jbertram <...@git.apache.org>
Subject [GitHub] activemq-artemis pull request #2035: [ARTEMIS-1819] Missing fields on listAl...
Date Wed, 25 Apr 2018 12:27:36 GMT
Github user jbertram commented on a diff in the pull request:

    https://github.com/apache/activemq-artemis/pull/2035#discussion_r184039273
  
    --- Diff: artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/ActiveMQServerControlImpl.java
---
    @@ -1849,7 +1860,9 @@ public String listSessionsAsJSON(final String connectionID) throws
Exception {
           try {
              List<ServerSession> sessions = server.getSessions(connectionID);
              for (ServerSession sess : sessions) {
    -            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID",
sess.getName()).add("creationTime", sess.getCreationTime()).add("consumerCount", sess.getServerConsumers().size());
    +            JsonObjectBuilder obj = JsonLoader.createObjectBuilder().add("sessionID",
sess.getName())
    +               .add("creationTime", sess.getCreationTime())
    --- End diff --
    
    I don't want to speak for @michaelandrepearce, but personally I find it much easier to
review PRs when all the changes in the PR are directly related. This PR is about adding missing
fields to some management methods not increasing code legibility. In general, increasing code
legibility is a good thing, but it can be detrimental in non-obvious ways (e.g. when lots
of legibility changes make it difficult to back-port fixes to maintenance branches) and IMO
changes which are exclusively for improving legibility should be in their own commits. Finally,
"legibility" can be subjective. You risk having your functional PR rejected by including non-functional,
subjective "improvements."


---

Mime
View raw message