Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return count when row_count is selected #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

colemanw
Copy link
Member

When the api is selecting only row_count it should return the count, otherwise the output looks like an empty array

When the api is selecting only row_count it should return the count, otherwise the output looks like an empty array
@colemanw
Copy link
Member Author

I haven't tested this, I'm not familiar with the cv build process, but in theory this should make a command like this actually return something useful:

cv api4 Contact.get +s row_count

else {
$this->sendResult($input, $output, $result);
}

return empty($result['is_error']) ? 0 : 1;
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how this part is relevant. Note that the result-code is important for handling errors in bash scripts. Returning 0 signals normal/successful exit. Errors should return non-zero. (Some tools go all-out and define various numeric-error-codes for different conditions. That's a bit of PITA here, which is why it merely returns 1.)

Some examples:

set -e
cd path/to/my/ext
git pull
cv upgrade:db
cd ..
set -e
cv api4 Contact.get +s display_name --out=list | while read NAME; do
   echo "Say hello to $NAME for me" 
done
cv flush && cv ext:list -R

In the first two cases, set -e means "If any command fails, stop execution and report the problem". If the third case, the && means "If left-command succeeds, then run right-command." They all rely on the exit-code.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just cleanup. This function looks to have been copy/pasted from the api3 function. Api3 returns an is_error key. Apiv4 does not.

@@ -149,11 +149,14 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$columns = empty($params['select']) ? array_keys($result->first()) : $params['select'];
$this->sendTable($input, $output, (array) $result, $columns);
}
elseif (!empty($params['select']) && $params['select'] === ['row_count']) {
$this->sendResult($input, $output, $result->count());
}
Copy link
Member

Choose a reason for hiding this comment

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

Aah, interesting, so you can select the row_count even though it's not actually a field in the result. This feels weird.... but OK, it appears to work w/AJAX. The AJAX-bindings wrap all results in an extra layer of JSON ({values: ..., count: ...}).

Of course, the patch above works if you select only the row_count. If you want to get data as well as row_count, then one needs to be more like the AJAX-bindings. I guess one could port the AJAX technique -- and for compatibility purposes, treat it as a flag, e.g.

cv api4 Contact.get +s display_name,row_count --wrap
{
  "count": 123,
  "values": [
    {first},
    {second}
  ]
}

or like:

cv api4 Contact.get +s row_count --header
{"count": 123}
[
  {first},
  {second}
]

FWIW, even with the wrapping, there's still a problem with chaining. You can see this in AJAX calls:

Screen Shot 2021-05-14 at 6 07 57 PM

I guess a couple questions:

  • Looking forward -- Are there more header/wrapper fields that we should expect?
  • Looking forward -- Do you expect some kind change/fix for the chaining issue?

If the answer is "No" on both fronts, then I'd be OK with this kind of special case. Otherwise, it's probably better to make the serialization formats more aligned.

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