Fixed Python packages table #4407
Conversation
fmanco
left a comment
There was a problem hiding this comment.
The solution here shouldn't be to remove the defaults but to allow the user to specify the path only if necessary/wanted. Keep the defaults for the case the user doesn't provide a path.
his reverts commit 639e3d5.
Reverting local changes
|
ok to test |
|
I think if no explicit directory is mentioned in the query then all the python packages should be searched at default locations such as '/usr/local/lib', '/usr/lib/' etc... and reported. |
fmanco
left a comment
There was a problem hiding this comment.
Thanks for the changes. I think you can refactor the main loop to make it more readable. Also some other nits.
| "/usr/lib/python2.7/site-packages/", | ||
| "/Library/Python/2.7/site-packages/", | ||
| }; | ||
| }; |
| return; | ||
| } | ||
|
|
||
|
|
| genPackage(path, r); | ||
| } else if (directory.find(".egg-info") != std::string::npos) { | ||
| auto path = directory + "/PKG-INFO"; | ||
| auto path = directory + "PKG-INFO"; |
There was a problem hiding this comment.
Why did you remove the / here and not above?
| #endif | ||
| } | ||
|
|
||
|
|
|
|
||
| for (const auto& key : kPythonPath) { | ||
| genSiteDirectories(key, results); | ||
| std::set<std::string> paths; |
There was a problem hiding this comment.
This code can be refactored to not repeat the genSiteDirectories() loop. Make paths be kPythonPath if no constraints and then have the genSiteDirectories() only once.
specs/python_packages.table
Outdated
| Column("license", TEXT, "License under which package is launched"), | ||
| Column("path", TEXT, "Path at which this module resides") | ||
| Column("path", TEXT, "Path at which this module resides"), | ||
| Column("directory", TEXT, "Directory where Python modules are located") |
There was a problem hiding this comment.
nit: missing trailing comma
|
ok to test |
|
@Rutwa189 thanks for the fixes. But you've indented a block of code. Can you please take a look? Thanks! |
|
@fmanco That was my fault I told her to do that to try and fix some other formatting issues and now we're wondering why code audit isn't catching this stuff. |
|
@fmanco I will fix the formatting. |
|
@obelisk yeah I wondered the same, but tests are passing... |
|
@Rutwa189 has updated the pull request. |
Solution to #4252 to enable users to provide a custom path for python packages.