Closed Gemba closed 2 months ago
Hi @Gemba, thank you for the PR.
I've taken a look and agree with the changes. I think though that the implementation of getting the list of parameters can be simplified a bit
_prepare_cli_params
to something like _get_cmdline_skyscraper
, it's more suggestive.$params
from this function directly instead of using a nameref assignment inside the method. We don't use this kind of assigment in RetroPie and it's kind of a 'gotcha' in Bash that not many are aware of - we prefer things to be simpler. Just return the array of params (as a string) from the cmdline assembly method and print it before usage in _scrape_skyscraper
.thank you.
Thanks for looking into it. I changed it to your suggestions. Works also well on my side.
However, I noticed something during testing that should not happen with an out-of-the-box installation of RP, but can happen when a user creates a platform/system folder with a space in its name. Then the passing of the system name (-p
) to Skyscraper breaks.
Edit: With the latest push I adressed that cornercase.
Yes, seems to work fine, thank you for the changes.
This goes back to the forum discussion a while ago: Within the scriptmodule there can be options persisted (cf.
Skyscraper.cfg
) which then get applied to the command line, which overrun settings in Skyscraper'sconfig.ini
.To add more visibility about the applied options (and to avoid confusion about different behaviour of Skyscraper from scriptmodule vs. Skyscraper CLI usage) the user gets the actual command echoed before the scraping starts.
Tested it well on my side and did not notice a regression.
Also, for some reasons two already present scraping modules did not made it into the scriptmodule.