Skip to content

Refactor API interactions in CLI#44

Open
RafsanNeloy wants to merge 2 commits into
aces:mainfrom
RafsanNeloy:Refactor
Open

Refactor API interactions in CLI#44
RafsanNeloy wants to merge 2 commits into
aces:mainfrom
RafsanNeloy:Refactor

Conversation

@RafsanNeloy
Copy link
Copy Markdown

Introduce utility functions for GET and POST requests, streamline error handling, and enhance session management. Update various data handling modules to utilize new API functions for improved readability and maintainability.

Signed-off-by: rafsanneloy <rafsanneloy@gmail.com>
@dlq
Copy link
Copy Markdown
Contributor

dlq commented Jun 1, 2026

This refactor appears to turn some validation failures into successful CLI exits. That is, an Error occurs but the CLI returns status 0 instead of non-zero.

Could we keep validation failures returning a non-zero exit status, either by having the data-layer functions return an error code/sentinel or by having handlers treat None from validation as failure?

@dlq
Copy link
Copy Markdown
Contributor

dlq commented Jun 1, 2026

In your description could you show what specific issues in plan.md you are addressing?

Signed-off-by: rafsanneloy <rafsanneloy@gmail.com>
@RafsanNeloy
Copy link
Copy Markdown
Author

Previously in capture_tests/expected_captures.txt has Failed: Invalid response from server line for command -

cbrain tag update 99 --name Renamed --user-id 2 --group-id 3

But it should be Tag 99 updated successfully!. and by this PR, cbrain-cli successfully generated after successful api hit. Previously as I'm not able to see the message using ./capture_tests/run_and_diff_captures command locally for that specific tag. that why the error occurs 😅

could you show what specific issues in plan.md you are addressing?

@dlq I'm trying to find and refactor the cbrain-cli codebase in such a way so that we can reduce repetitive codes and fixing bugs. Also cbrain-cli mainly calls the cbrain backend server using E.g. get, put, delete etc. For example, Previously the data called by the file show endpoint are like this -

def show_file(args):
   ......
    userfile_endpoint = f"{cbrain_url}/userfiles/{file_id}"
    headers = auth_headers(api_token)

    request = urllib.request.Request(userfile_endpoint, data=None, headers=headers, method="GET")

    with urllib.request.urlopen(request) as response:
        data = response.read().decode("utf-8")
        file_data = json.loads(data)

    return file_data

But now i updated the function, with by calling the api_get like this -

def show_file(args):
   ....
    return api_get(f"{cbrain_url}/userfiles/{file_id}", api_token)

the get function is -

def api_get(url, token, params=None):
    if params:
        url = f"{url}?{urllib.parse.urlencode(params)}"
    req = urllib.request.Request(url, headers=auth_headers(token), method="GET")
    with urllib.request.urlopen(req) as r:
        return json.loads(r.read().decode())

Did you see that, by using a common get function we can remove the extra lines from show file and we can call the api where we need. As this cli project will be improving day by day, I believe we really want that transformation before working on the plan.md. What do you think @dlq ?

@dlq
Copy link
Copy Markdown
Contributor

dlq commented Jun 2, 2026

Thanks, this tag update fix makes sense to me. Nice catch. It looks like the old command was actually updating the tag successfully, then tripping afterward because the successful response was empty/non-JSON. Updating the capture for that behavior seems reasonable, and it would be good to call that out explicitly in the PR description.

I do think there is still one important issue to fix before this can merge, though. On the latest head, invalid commands like:

cbrain file list --per-page 4
cbrain dataprovider list --per-page 4
cbrain task list bourreau-id

still return status 0. A couple of them also print confusing follow-up output like No data providers found after the validation error. So the original validation/exit-code concern is still alive.

For the plan.md mapping: this PR is mainly aiming at #25: centralize request handling, which is a good direction. The tricky part is that #25 also needs a clearer request/handler contract and tests, otherwise broad cleanup can accidentally change CLI behavior.

Could you either fix the validation contract in this PR, or narrow the PR to the tag update / empty-response fix plus a smaller API-helper migration with tests? That would make this much easier to review and get merged.

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.

2 participants