fix: remove return from finally block to prevent exception swallowing

[themavik]

Summary

The get_log_path function in elementary/cli/cli.py had a return statement inside a finally block, which silently swallows all in-flight exceptions — including BaseException subclasses like KeyboardInterrupt.

This means that if ctx.args doesn't contain --target-path (the common case), the resulting ValueError from list.index() is silently suppressed. More critically, if an unexpected exception like KeyboardInterrupt or SystemExit is raised during the try block, it would also be swallowed, making the CLI unresponsive to Ctrl+C.

Changes

• Replaced the try/finally with a proper try/except block that catches only the expected exceptions (ValueError from list.index() and IndexError from accessing ctx_args[i + 1])
• Moved the directory creation (os.makedirs) and return statement outside the exception handler
• The fallback behavior (using Config.DEFAULT_TARGET_PATH) is preserved

References

• Python docs on finally behavior
• Fixes return in finally swallows exceptions #1731

Assisted by Claude Opus 4.6 max thinking

Made with Cursor

Summary by CodeRabbit

• Refactor
  • Improved error handling in CLI logging path resolution for better reliability and consistency.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors exception handling in the get_log_path() function by replacing a finally block containing a return statement with explicit try/except logic. Catches ValueError and IndexError to fall back to default path behavior, eliminating exception suppression anti-pattern.

Changes

Cohort / File(s)	Summary
Exception Handling Fix elementary/cli/cli.py	Replaced finally block with explicit except clauses to catch ValueError and IndexError, removing the exception-suppressing return statement from the finally block.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Poem

🐰 A fix for a tricky Python trick—
No more returns in finally quick,
Exceptions now flow as they should,
Error handling clear and good,
Our code hops forward, understood! 🌟

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: removing a return statement from a finally block to prevent exception swallowing, which aligns with the primary objective of the PR.
Linked Issues check	✅ Passed	The PR directly addresses issue #1731 by removing the return from the finally block, explicitly catching ValueError and IndexError, and allowing normal exception propagation as requested.
Out of Scope Changes check	✅ Passed	All changes are scoped to the get_log_path function in elementary/cli/cli.py and directly address the exception handling issue without introducing unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

👋 @themavik
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.