Loading ...
Sorry, an error occurred while loading the content.

Re: [pcgen_developers] CI Upgrade and Code Checks

Expand Messages
  • James Dempsey
    Hi, I think wiki voting is the best option for variable naming, but for things like inline conditionals which we have always used I m happy to drop the rule
    Message 1 of 12 , May 5, 2011
    • 0 Attachment
      Hi,

      I think wiki voting is the best option for variable naming, but for things like inline conditionals which we have always used I'm happy to drop the rule (which I will do shortly). We can also vary the severity of various classes of issues too.

      I'm a fan of the constants in upper case with underscores rule and anything which highlights Hungarian notation for fixing is good IMO. While the 34000 issues might seem a lot, we are talking about quite a lot of code.

      Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.

      Cheers,
      James.

      On 5/05/2011 5:08 PM Martijn Verburg wrote
      Hi All,

      I firmly believe we should vote on each of these issues (by simple Wiki polling as it were).

      Also, do we need to be careful about applying formatting changes whilst we still have a major branch to merge in?

      K

      On 5 May 2011 00:24, Andrew Wilson <andrew@...> wrote:
       
      On 1 May 2011 23:15, James Dempsey <jdempsey@...> wrote:
      PS: Are there any checkstyle rule changes anyone thinks would be useful?
       
      Hi James
       
      There are some things it's throwing up that I don't think are useful.
       
      It has an irrational fear of inline conditionals, we should probably
      disable that.
       
      It's flagging up names that don't match some standard that we don't
      seem to adhere to at all.  It wants constants to be all uppercase, with
      words separated by underscores.  It also complains when a symbol
      that isn't a constant uses an underscore.  Are we sure we want to
      adopt these?
       
      andrew
    • Andrew Wilson
      ... I don t mind upper case with underscores for constants. the thing that gave me pause was it complained about the use of underscores in something tht
      Message 2 of 12 , May 5, 2011
      • 0 Attachment
        On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
        I'm a fan of the constants in upper case with underscores rule and    anything which highlights Hungarian notation for fixing is good IMO.    While the 34000 issues might seem a lot, we are talking about quite    a lot of code.
         
        I don't mind upper case with underscores for constants.  the thing that gave me pause was
        it complained about the use of underscores in something tht wasn't a constant.  it was one
        of a series of symbols where we have multiple of the same thing but for different bits of
        the GUI
         
         private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
         private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
         private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
         private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
        do we want to eliminate the underscores to give us
         
        ClassTabAvailableListMode
        ClassTabSelectedListMode
        EquipTabAvailableListMode
        EquipTabSelectedListMode
         
        Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
        Does that mean I should stop doing this 'til after you've merged in?
         
        andrew
      • James Dempsey
        Hi Andrew, On 5/05/2011 11:48 PM Andrew Wilson wrote ... Well if the first character were lower-case (probably your email editor being helpful) they would
        Message 3 of 12 , May 6, 2011
        • 0 Attachment
          Hi Andrew,

          On 5/05/2011 11:48 PM Andrew Wilson wrote
          On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
          I'm a fan of the constants in upper case with underscores rule and    anything which highlights Hungarian notation for fixing is good IMO.    While the 34000 issues might seem a lot, we are talking about quite    a lot of code.
           
          I don't mind upper case with underscores for constants.  the thing that gave me pause was
          it complained about the use of underscores in something tht wasn't a constant.  it was one
          of a series of symbols where we have multiple of the same thing but for different bits of
          the GUI
           
           private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
           private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
           private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
           private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
          do we want to eliminate the underscores to give us
           
          ClassTabAvailableListMode
          ClassTabSelectedListMode
          EquipTabAvailableListMode
          EquipTabSelectedListMode
          Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

          All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.

           
          Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
          Does that mean I should stop doing this 'til after you've merged in?
           
          I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.

          Cheers,
          James.
        • Andrew Wilson
          ... Ok, I ve never had the pleasure of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get
          Message 4 of 12 , May 7, 2011
          • 0 Attachment
            On 7 May 2011 02:45, James Dempsey <jdempsey@...> wrote:
             private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
             private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
             private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
             private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
            do we want to eliminate the underscores to give us
             
            ClassTabAvailableListMode
            ClassTabSelectedListMode
            EquipTabAvailableListMode
            EquipTabSelectedListMode
             
            Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

            All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.
                
            Ok, I've never had the "pleasure" of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get onboard with that. 
            Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
            Does that mean I should stop doing this 'til after you've merged in?
             
            I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.
              
            I have some changes in the core.  It took a bit of faffing about, but I've finally got idea to highlight the checkstyle problems as real time checks as I'm working on a file and that makes them a bit easier to deal with.
             
            andrew
          • Martijn Verburg
            As a reminder to all developers, you can vote for the checkstyle and other coding standards here: http://wiki.pcgen.org/Coding_Standards The voting is only
            Message 5 of 12 , May 7, 2011
            • 0 Attachment
              As a reminder to all developers, you can vote for the checkstyle and other coding standards here:


              The voting is only open to those who have committed code in the past 6 months

              Feel free to add new rules etc that we can all vote on

              K

              On 7 May 2011 12:51, Andrew Wilson <andrew@...> wrote:
               

              On 7 May 2011 02:45, James Dempsey <jdempsey@...> wrote:
               private static int classTab_AvailableListMode = GuiConstants.INFOCLASS_VIEW_NAME;
               private static int classTab_SelectedListMode = GuiConstants.INFOCLASS_VIEW_NAME;
               private static int equipTab_AvailableListMode = GuiConstants.INFOEQUIPPING_VIEW_EQUIPPED;
               private static int equipTab_SelectedListMode = GuiConstants.INFOEQUIPPING_VIEW_NAME;
              do we want to eliminate the underscores to give us
               
              ClassTabAvailableListMode
              ClassTabSelectedListMode
              EquipTabAvailableListMode
              EquipTabSelectedListMode
               
              Well if the first character were lower-case (probably your email editor being helpful) they would match the usual standard yes. I've spent a lot of time in the GUI code and haven't spent the effort to change it, so I'm obviously not too stressed :) I think I have excised most of the m_Foo style entries. They give me bad flashbacks to my early days in the Cobol mines!

              All that said, voting on these issues is a good thing so we get a consensus view and make the code environment better for all of us to work in.
                  
              Ok, I've never had the "pleasure" of doing Cobol, but if we want to eliminate all the single character at the start followed by an underscore I can get onboard with that. 
              Good point about the reformatting. Localised changes should be ok, but bulk changes, particularly of white-space and other layout will cause us much grief I would expect.
              Does that mean I should stop doing this 'til after you've merged in?
               
              I might do a scan and let you know where is safe and where we should be careful. In general the core and the plugins are fine to work on, the GUI is probably wasted effort at this stage and the loading stuff is being changed so it would be best to avoid.
                
              I have some changes in the core.  It took a bit of faffing about, but I've finally got idea to highlight the checkstyle problems as real time checks as I'm working on a file and that makes them a bit easier to deal with.
               
              andrew


            • Andrew Wilson
              Right, now I ve done a few of them I have some other points. I reckon there s 9 man months to a year s worth of effort involved in cleaning these up :-( One
              Message 6 of 12 , Jun 2, 2011
              • 0 Attachment
                Right, now I've done a few of them I have some other points.
                 
                I reckon there's 9 man months to a year's worth of effort involved in cleaning
                these up :-( One full day at the weekend every weekend for five years.  Yikes!
                Just sayin' 's'all.
                 
                One of the files I'm working on is cmplaining about an empty catch block.  Thing
                is it's commented, so someone's put a bit of thought into it.  Do we really care
                about this?  if not how do I get rid of the warning.  do we have a supression
                mechanism that we're using?
                andrew
                 
                On 5 May 2011 12:07, James Dempsey <jdempsey@...> wrote:
                Hi,

                I think wiki voting is the best option for variable naming, but for things like inline
                conditionals which we have always used I'm happy to drop the rule (which I will
                do shortly). We can also vary the severity of various classes of issues too.
              • James Dempsey
                Hi Andrew, On 3/06/2011 4:04 AM Andrew Wilson wrote ... Thanks for that - it has been great to see the clean-up progressing. ... Yes there is little doubt that
                Message 7 of 12 , Jun 6, 2011
                • 0 Attachment
                  Hi Andrew,

                  On 3/06/2011 4:04 AM Andrew Wilson wrote
                  Right, now I've done a few of them I have some other points.
                  Thanks for that - it has been great to see the clean-up progressing.

                   
                  I reckon there's 9 man months to a year's worth of effort involved in cleaning
                  these up :-( One full day at the weekend every weekend for five years.  Yikes!
                  Just sayin' 's'all.
                  Yes there is little doubt that it is a big job. One primary benefit of setting up the violations plugin is that we can ensure it doesn't get any worse and instead gradually improve the situation.

                   
                  One of the files I'm working on is cmplaining about an empty catch block.  Thing
                  is it's commented, so someone's put a bit of thought into it.  Do we really care
                  about this?  if not how do I get rid of the warning.  do we have a supression
                  mechanism that we're using?

                  One technique I have used in the past is to add a log entry to the catch block. Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning is warranted IMO.

                  Cheers,
                  James.
                • Andrew Wilson
                  ... Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning ... warranted IMO. ... Fair enough, what severity of thing should I log it as?
                  Message 8 of 12 , Jun 6, 2011
                  • 0 Attachment
                    On 6 June 2011 23:08, James Dempsey <jdempsey@...> wrote:
                    > One of the files I'm working on is complaining about an empty catch block.  Thing 
                    > is it's commented, so someone's put a bit of thought into it.  Do we really care 
                    > about this?  if not how do I get rid of the warning.  do we have a suppression 
                    > mechanism that we're using?

                    One technique I have used in the past is to add a log entry to the catch block. 
                    Generally ignoring exceptions is a bad idea so the PMD/Checkstyle warning is 
                    warranted IMO.

                    Fair enough, what severity of thing should I log it as? 

                    andrew
                  Your message has been successfully submitted and would be delivered to recipients shortly.