Skip to content

Conversation

axif0
Copy link
Collaborator

@axif0 axif0 commented Aug 24, 2025

Tokens work as long as the server says they work.

Enhance error handling and refactor API request logic across multiple modules.
Improved HTTP error responses in cli_utils.py, streamlined data fetching in data modules, and removed redundant error handling.
Updated command argument parsing in main.py to ensure required fields are enforced.
Cleaned up interactive prompts in tags.py and adjusted formatting in output functions.

Fixes: #5

@axif0 axif0 changed the title client-side token expiration logic Enhance error handling and refactor API request logic Aug 25, 2025
@axif0 axif0 requested a review from MontrealSergiy August 25, 2025 21:57
@axif0 axif0 linked an issue Aug 25, 2025 that may be closed by this pull request
@axif0
Copy link
Collaborator Author

axif0 commented Aug 28, 2025

Is the error formatting ok?

image

@MontrealSergiy
Copy link
Contributor

MontrealSergiy commented Aug 28, 2025

In the first line looks like there the message is caused by is a bug in CBRAIN, IMHO you should make clear that error message is from CBRAIN and not your client.

I submitted a PR for that CBRAIN bug a few days ago, and it should be reviewed soon, but something like that potentially can happen again when new bug is introduced or discovered. Maybe add a prefix 'Server error:', 'API Endpoint encountered error:' @prioux @dlq @natacha-beck ?

In the second line the message seems tractable, IMHO

Copy link
Contributor

@MontrealSergiy MontrealSergiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on your example, I think even if we extract the detailed server message, we should still communicate the status code. More, for less savvy users, we can also provide a short description of what it means, something akin

Server error (5xx): ...
Validation error (422): ...

@axif0
Copy link
Collaborator Author

axif0 commented Aug 29, 2025

Oh, do you want to add status code like -

Server error (5xx): Error: ActiveRecord:: Record Not Found
                      In UserfilesController#show Couldn't find Userfile with 'id'=230

@MontrealSergiy
Copy link
Contributor

Not sure I think Record not Found is 404 ( a client/user side error). All the 5xx (500 - 505) codes are some server side errors. Can we keep this distinction?

@axif0
Copy link
Collaborator Author

axif0 commented Aug 31, 2025

Improve the error display -
image

Copy link
Contributor

@MontrealSergiy MontrealSergiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I preferred the older, more user-tailored error messages, but this way - returning only server errors - might be easier to maintain and works well for advanced users.

There are a few cases where a server error message isn’t returned, but that doesn’t bother me much.

I do feel that sometimes the wording could be slightly improved. For example, instead of:
Try with authorized access
it would be clearer to show:
Error: Access denied. Please log in using authorized credentials.

@MontrealSergiy MontrealSergiy merged commit f61efcb into aces:main Sep 2, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output should be textual unless json(l) is requested Token expiry is hardcoded to 1 day
2 participants