More refactoring of Python code to address Sonar issues#906
Conversation
| if file.name in prop_files: | ||
| process = self.read_drive_properties(f"{CFG_DIR}/{file.name}.{PROPERTIES_SUFFIX}") | ||
| process = self.read_drive_properties( | ||
| Path(CFG_DIR) / f"{file.name}.{PROPERTIES_SUFFIX}" |
There was a problem hiding this comment.
Wow, this is super weird at first sight (operator overloading) but cool to see in practice.
python/web/src/web_utils.py
Outdated
| if ( | ||
| # Note: This check will fail if the wireless interface doesn't start with wlan | ||
| interface.startswith("wlan") and | ||
| sys_cmd.introspect_file("/etc/sysctl.conf", r"^net\.ipv4\.ip_forward=1$") and | ||
| Path("/etc/iptables/rules.v4").is_file() | ||
| or sys_cmd.introspect_file( | ||
| "/etc/dhcpcd.conf", | ||
| r"^denyinterfaces " + interface + r"$", | ||
| ): | ||
| status = False | ||
| return_msg = _("Configure the network bridge before using a wired network device.") | ||
| elif not Path("/etc/network/interfaces.d/rascsi_bridge").is_file(): | ||
| status = False | ||
| return_msg = _("Configure the network bridge before using a wired network device.") | ||
|
|
||
| return {"status": status, "msg": return_msg + f" ({interface})"} | ||
| ) | ||
| and Path("/etc/network/interfaces.d/rascsi_bridge").is_file() | ||
| ): |
There was a problem hiding this comment.
I think this method's structure is worth another look because the change obfuscates the intent to a greater degree than the existing code. There's quite a lot going on and I found it difficult to grok when merged into a single expression.
There's also a loss of verbosity. If we want to simply the messaging in the web UI, then I think moving those messages to the logs should be considered.
There was a problem hiding this comment.
Actually, let me revert this change and add a TODO instead. I shouldn't be changing localized strings this close to the next release.
| return response(message=[ | ||
| (_("Image file created: %(file_name)s", file_name=full_file_name), "success"), | ||
| (process["msg"], "success"), | ||
| ]) |
There was a problem hiding this comment.
(Optional) We should try to have one message per operation if possible.
There was a problem hiding this comment.
Agreed. I can add TODOs here. Once we've tagged the next release we can refactor these status messages. I don't want to mess with localized strings this close to the release.
This reverts commit 22a1bf1.
|
SonarCloud Quality Gate failed. |
Uh oh!
There was an error while loading. Please reload this page.