Added machine policy exception handling to kde_applier #211
Conversation
|
With the previous commits, I made a few changes in accordance with pylint's recommendations and fixed some minor bugs |
|
Regarding this pull request, my comments are the same as with the others: functional code fixes should be separated from refactoring changes into different pull requests. This makes it easier to discuss and make decisions about each type of change. |
Done |
| logdata = dict({"error": str(exc)}) | ||
| log("E31", logdata) | ||
| else: | ||
| pass |
There was a problem hiding this comment.
Unfortunately, I must decline the proposed changes in this commit, as the mass replacement of single quotes (') with double quotes (") without functional improvements creates unnecessary noise in the git history and makes it difficult to track significant changes in the future. Additionally, this change does not align with the established coding culture and conventions of this project, where the existing quoting style has been consistently applied throughout its development history.
| getattr(dbus_iface, config['dbus_method'])(*config['dbus_args']) | ||
| else: | ||
| getattr(dbus_iface, config['dbus_method'])() | ||
| except dbus.exceptions.DBusException as e: | ||
| except dbus.exceptions.DBusException as exc: | ||
| logdata = dict({'error': str(exc)}) | ||
| log('E31', logdata) | ||
| else: |
| # You should have received a copy of the GNU General Public License | ||
| # along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
|
||
| from .applier_frontend import applier_frontend, check_enabled | ||
| from util.logging import log | ||
| from util.util import get_homedir | ||
| from util.exceptions import NotUNCPathError | ||
| import os | ||
| import subprocess | ||
| import re | ||
| import dbus | ||
| import shutil | ||
| from util.logging import log | ||
| from util.util import get_homedir | ||
| from util.exceptions import NotUNCPathError | ||
| from util.dbus import dbus | ||
| from .applier_frontend import applier_frontend, check_enabled | ||
|
|
||
| class kde_applier(applier_frontend): | ||
| __module_name = 'KdeApplier' |
There was a problem hiding this comment.
I suggest making these changes as a separate pull request, modifying these code patterns throughout the entire project in accordance with canonical standards. It's better not to include such changes in the same PR with structural code modifications.
| logdata['exc'] = exc | ||
| else: | ||
| log('D199') | ||
|
|
There was a problem hiding this comment.
I don't think we should wrap the main function call in a try-except block. This approach loses the contextual information about where exactly the exception occurred and makes debugging significantly harder.
Instead, it's better to handle exceptions at specific boundary points in the code where we can provide meaningful error messages and take appropriate recovery actions. This way, when an issue occurs, we can quickly identify the exact location and reason for the exception, rather than having all errors caught at the top level.
2 participants
Errors may occur when running apply for machine policy:
2025-04-16 12:18:38.807|[E00024]| Error occured while running machine applier|{'applier_name': 'kde', 'msg': “name ‘exc’ is not defined”}
Exception needs to be handled