Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-4513

Quoting of paths in doxygen configuration files breaks makeDocs

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: lsstDoxygen, sconsUtils
    • Labels:
      None
    • Story Points:
      4
    • Team:
      Architecture

      Description

      In DM-3200 I modified sconsUtils such that all the paths used in doxygen.conf files were quoted so that spaces could be used. This change broke documentation building (DM-4310) because makeDocs did not expect double quotes to be relevant. This ticket is to fix makeDocs and to re-enable quoting of paths in config files.

        Attachments

          Issue Links

            Activity

            Hide
            tjenness Tim Jenness added a comment - - edited

            I have changed makeDocs such that it understands quoting in doxygen.conf file and will now quote tho output config file if required. Quoting has been added back to sconsUtils but only if required by the presence of a space.

            The DM-4513 sconsUtils branch is being tested on Jenkins. The only way to test the full buildbot system was to merge the lsstDoxygen fixes directly to master as buildbot will not checkout a ticket branch.

            Show
            tjenness Tim Jenness added a comment - - edited I have changed makeDocs such that it understands quoting in doxygen.conf file and will now quote tho output config file if required. Quoting has been added back to sconsUtils but only if required by the presence of a space. The DM-4513 sconsUtils branch is being tested on Jenkins. The only way to test the full buildbot system was to merge the lsstDoxygen fixes directly to master as buildbot will not checkout a ticket branch.
            Hide
            tjenness Tim Jenness added a comment -

            Would you mind reviewing please? The lsstDoxygen changes have already been merged as that was the only way to test them but I'm happy for code review on that code as well as sconsUtils.

            Show
            tjenness Tim Jenness added a comment - Would you mind reviewing please? The lsstDoxygen changes have already been merged as that was the only way to test them but I'm happy for code review on that code as well as sconsUtils .
            Hide
            rowen Russell Owen added a comment -

            Overall this looks like a significant improvement.

            I do have a few requests

            lsstDoxygen

            • Move the code that splits path-like data to a separate function, since it can then be tested and it might prove useful elsewhere someday
            • Add a unit test for that function

            sconsUtils

            • Consider moving _quote_path and _quote_paths up one level as public functions
            • For every call to _quote_paths you join the results using a space; consider moving that into _quote_paths or a separate function that calls it
            Show
            rowen Russell Owen added a comment - Overall this looks like a significant improvement. I do have a few requests lsstDoxygen Move the code that splits path-like data to a separate function, since it can then be tested and it might prove useful elsewhere someday Add a unit test for that function sconsUtils Consider moving _quote_path and _quote_paths up one level as public functions For every call to _quote_paths you join the results using a space; consider moving that into _quote_paths or a separate function that calls it
            Hide
            tjenness Tim Jenness added a comment -

            I have realised that the parsing in makeDocs can be handled by the standard csv package so I removed the bespoke parsing code and replaced it with two lines that simply specify the delimiter and quote character. This seems to work fine. The changes are in a different branch: tickets/DM-4513b.

            I have modified the sconsUtils patch such that _quote_paths returns a string that has been space-joined. This is better than doing the join each time. I don't want to move the support functions up a level as I'm not sure I want to pollute the builders namespace with something that is not going to be needed elsewhere and is currently doxygen-specific.

            Please take a look at the new versions.

            Show
            tjenness Tim Jenness added a comment - I have realised that the parsing in makeDocs can be handled by the standard csv package so I removed the bespoke parsing code and replaced it with two lines that simply specify the delimiter and quote character. This seems to work fine. The changes are in a different branch: tickets/ DM-4513 b . I have modified the sconsUtils patch such that _quote_paths returns a string that has been space-joined. This is better than doing the join each time. I don't want to move the support functions up a level as I'm not sure I want to pollute the builders namespace with something that is not going to be needed elsewhere and is currently doxygen-specific. Please take a look at the new versions.
            Hide
            rowen Russell Owen added a comment - - edited

            Your changes look great. Using csv is a nice improvement.

            Two minor suggestions:

            1) You can tell csv.reader to skip multiple separators to further simplify the code, e.g.:

            path_list = next(csv.reader([entries], delimiter=' ', quotechar='"', skipinitialspace=True))
            entries = sep.join(path_list)
            

            2) In quote_path I suggest quoting all paths so that the output is more predictable. But your way works fine and it's your call.

            Show
            rowen Russell Owen added a comment - - edited Your changes look great. Using csv is a nice improvement. Two minor suggestions: 1) You can tell csv.reader to skip multiple separators to further simplify the code, e.g.: path_list = next(csv.reader([entries], delimiter=' ', quotechar='"', skipinitialspace=True)) entries = sep.join(path_list) 2) In quote_path I suggest quoting all paths so that the output is more predictable. But your way works fine and it's your call.
            Hide
            tjenness Tim Jenness added a comment -

            Thanks for the skipinitialspace idea. That does simplify things even more. I have now merged this. I have retained the dynamic addition of the double quotes.

            Show
            tjenness Tim Jenness added a comment - Thanks for the skipinitialspace idea. That does simplify things even more. I have now merged this. I have retained the dynamic addition of the double quotes.

              People

              Assignee:
              tjenness Tim Jenness
              Reporter:
              tjenness Tim Jenness
              Reviewers:
              Russell Owen
              Watchers:
              Russell Owen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.