Quoting of paths in doxygen configuration files breaks makeDocs

XMLWordPrintable

Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• 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.

Activity

Hide
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
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
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
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
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
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
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
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
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
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
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
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:
Tim Jenness
Reporter:
Tim Jenness
Reviewers:
Russell Owen
Watchers:
Russell Owen, Tim Jenness