Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to discourage the installation of JetBrains IDEs via Flatpak, instead guiding users toward the JetBrains Toolbox. It adds a new hook configuration in bazaar.yaml, modifies the blocklist.yaml to allow the hook to intercept these installations, and introduces a Python script hooks.py to handle the logic. Review feedback identified a critical bug in how terminal commands are spawned, a potential AttributeError when environment variables are missing, and a recommendation to avoid bare exception handlers.
| subprocess.Popen(args, start_new_session=True, stdout=subprocess.DEVNULL) | ||
|
|
||
| def spawn_ujust(id): | ||
| spawn_and_detach(['flatpak-spawn', '--host', 'xdg-terminal-exec', '-x', f'ujust {id}']) |
There was a problem hiding this comment.
The command and its arguments are being passed as a single string f'ujust {id}' to xdg-terminal-exec. This is likely to fail because the shell will try to execute a program named "ujust install-jetbrains-toolbox" instead of the ujust command with install-jetbrains-toolbox as an argument. The command and its arguments should be passed as separate elements in the list.
| spawn_and_detach(['flatpak-spawn', '--host', 'xdg-terminal-exec', '-x', f'ujust {id}']) | |
| spawn_and_detach(['flatpak-spawn', '--host', 'xdg-terminal-exec', '-x', 'ujust', id]) |
| def handle_jetbrains(): | ||
|
|
||
| def appid_is_jetbrains(appid): | ||
| return appid.startswith('com.jetbrains.') |
There was a problem hiding this comment.
The appid variable, which comes from os.getenv('BAZAAR_TS_APPID'), can be None if the environment variable is not set. Calling .startswith() on None will raise an AttributeError, causing the script to crash. You should add a check to ensure appid is not None before calling string methods on it.
| return appid.startswith('com.jetbrains.') | |
| return appid is not None and appid.startswith('com.jetbrains.') |
| case 'action': | ||
| try: | ||
| spawn_ujust('install-jetbrains-toolbox') | ||
| except: |
There was a problem hiding this comment.
Using a bare except: is generally discouraged as it catches all exceptions, including system-exiting ones like SystemExit or KeyboardInterrupt. This can hide bugs and make the program difficult to interrupt or debug. It's better to catch a more specific exception, like Exception.
| except: | |
| except Exception: |
1 participant
It would be nice to add Visual Studio Cod{e/ium} / Google Studio support too before merge
About issue #240