search

roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.03k stars 205 forks source link

GH-512 python: % -> format(); other small refactoring #513

Closed adrianopol closed 1 year ago

gavv commented 1 year ago

I most cases where %s was replaced with +, I find the new version is less readable.

For example here:

'-Wl,-soname,libroc%s' % subenvs.public_libs['SHLIBSUFFIX']

I immediately see that the resulting string is "libroc", but here:

'-Wl,-soname,libroc' + subenvs.public_libs['SHLIBSUFFIX']

I need to read the whole expression to see what resulting string looks like.

Please consider using format instead of + in such cases.

This looks OK:

'-Wl,-soname,libroc{}'.format(subenvs.public_libs['SHLIBSUFFIX'])
gavv commented 1 year ago

In many (most?) places where / was replaced with os.path.join, it was done incorrectly.

When we pass paths to scons (e.g. to CPPPATH), IIRC we should use /.

Anyway, let's avoid this change in this PR, this is out of its scope.

github-actions[bot] commented 1 year ago

:umbrella: The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

gavv commented 1 year ago

LGTM