apache
refactor(cli): Improve CLI commands with better error handling and output formatting
What changes were proposed in this pull request?
This PR enhances multiple CLI commands by improving error handling, resource management, and output formatting. The following changes were made:
- Improved exception handling for better debugging and user feedback.
- Enhanced error messages for missing entities to provide clearer guidance.
- Implemented try-with-resources for better resource management where applicable.
- Ensured proper handling of empty responses and formatted outputs for better readability.
- Standardized approach to handling CLI command execution errors.
Why are the changes needed?
These improvements ensure that CLI commands provide more reliable feedback to users, prevent unhandled exceptions, and improve resource management. The changes also enhance maintainability and debugging for developers.
Fix: #6528
Does this PR introduce any user-facing change?
Yes, the following user-facing changes are introduced:
- Improved error messages for missing entities.
- Better formatting for command outputs.
- Safer resource handling to prevent unexpected failures.
@justinmclean it was taking too long for me as there where multiple files but finally made my way, now it's up to you, you may go through the changes and feel free to mention also added comments where i feel it should be there for better readability.
@justinmclean @tengqm tried to fix and resolve as suggested can just go through it ones also removed what i feel is not needed.
The CI is failing as we use spotless to make sure the code has consistent formatting. You'll need to run ./gradlew :clients:cli:spotlessApply and check in the changes to fix this.
@justinmclean tried to fix all return statement, using it was my best practice but for this repo as u said we don't generally need so removed apart from that fixed all if statements outside the try block. Also thankyouuu! for giving reviews always as there are multiples files so this is taking way more time.
@tengqm made relevant changes.
Two points here:
- If you don't agree to a feedback, you can leave it there, but don't mark it as "resolved". Marking a feedback as "resolved" without changing the code accordingly means "I don't care" or "I'm ignoring what you suggested.".
- For the "return" statement, I would say that again. I don't have a strong opinion on whether we should add it. But if there is decision, we need to be consistent in the code.
@tengqm Apologies for any confusion. As Justin mentioned, the decision was to remove the return statement, and I’ve tried to remove it from almost all files. I didn’t mean to take your feedback the wrong way—I truly appreciate the review. I might have missed checking all files since I’m currently in the middle of my university exams while still staying active in open-source contributions.
I’ll make sure to correct it properly. Would it be fine if I just remove the return statement from all files to ensure consistency?
@tengqm Just to confirm, should I go ahead and remove the return statement from all cases, including if statements?
@tengqm Just to confirm, should I go ahead and remove the return statement from all cases, including if statements?
Yup. If the decision was that 'return' is unnecessary, please remove them all.
@Brijeshthummar02, there are still some outstanding, unaddressed comments. You have also resolved several issues without fixing them. It's getting there, but I think it still needs a little work.
My main concern is that the added try (GravitinoClient client = buildClient(metalake)) will close the client too soon, our unit tests are unlikely to catch this. I'll take a look on Monday to see if this is an issue.
@Brijeshthummar02, there are still some outstanding, unaddressed comments. You have also resolved several issues without fixing them. It's getting there, but I think it still needs a little work.
My main concern is that the added
try (GravitinoClient client = buildClient(metalake))will close the client too soon, our unit tests are unlikely to catch this. I'll take a look on Monday to see if this is an issue.
Follow up As you mentioned you will take a look and point out issue if exist.